Re-write of PDF invoices from user input, decoupling of Invoice from FPDF class

Question

Following on from this post and taking on board some of the suggestions, what follows is a re-write of the original python code. It's also just a rewrite of some of the functionality, this code only deals with the invoice data and turning it into a PDF, rather than collecting/inputting the invoice data then turning it into an invoice. If there's too much code, here's my defence ;-)

In summary: data => PDF

In my original post @Kate said (approximately) that I should possibly use I library such as Jinja. This brings me onto the essential subject of this post: Complexity.

Having read this post on decoupling and then this one The TL;DR is that decoupling introduces complexity and there's a fine line. This code is only for my invoices, there'll only ever be one 'style'. But then there's good coding practice, any tips on balancing quick and dirty versus/elegant ?

The full invoice looks something like this:

However, in order to reduce the amount of code that needs to be looked at (in line with comments from previous post) for this post, this is the invoice I've coded for: This invoice contains an example of each abstraction I've written, class Table and class TextBlock. The Table, I hope, is self explanatory, a TextBlock is several lines of text with roughly the same characteristics, eg: font style or positioning within the page.

However, there's an 'however' to my however (nested 'however's ? ) I've also included unittests for the code, which has doubled the length of the code, however that was also a suggestion from the previous post.

0) How are my unit tests ?

The invoice is represented by this module: invoice.py

from dataclasses import dataclass

@dataclass
class DailyJobData:
    hours: float = 0.0
    TA_for_this_day: bool = True

'''
Due to the reduced nature of the code base for this code review post only
Invoice['Job_Constants'] & Invoice['Data'] will actually be used.
'''

Invoice = {
    'Constants' : {
        'business_name' : 'Quizz Tramarais',
        'name' : 'Lozminda Lamarais',
        'address' : '3rd floor flat, Whistle Street, Oldport, OP4 5GH',
        'telephone' : '07889 578685',
        'my_refs' : {
            'national_ins' : 'DF 56 57 48 C',
            'UTR' : '345865'
        }
    },
    'Date' : {
        'year' : 2023,
        'month_last_day' : 6,
        'month' : 'June'
    },
    'Job_Constants': {
        'name' : 'Unit 6',
        'job_address' : 'East Winsley Street',
        'city' : 'Branburn',
        'ref' : 'XYZ 008',
        'travel_allowance' : 25.00
    },
    'Data' : {
        1 : DailyJobData(10),
        8 : DailyJobData(8, False),
        2 : DailyJobData(8.5),
        3 : DailyJobData(8.25),
        9 : DailyJobData(8, False)
    }
}

I'm using FPDF1, and have therefore had to 'roll my own' Table function. I've also used a Table and TextBlock class as abstractions to reduce coupling between the Invoice dictionary and the PDF class used to create the pdf.

1) Is dataclass for Table appropriate here. I've left TextBlock as an ordinary class (for comparison), should both be dataclasses or neither ?

Table and TextBlock classes in tables_and_text.py

# python3 -m unittest -v tables_and_text
import unittest
from dataclasses import dataclass
from typing import Dict, Union, List, Tuple

@dataclass
class Table:
    '''
    xpos                int: postion from left margin in mm.
    ypos                int: postion from top margin in mm.
    data                list: the contents of the table including the table
                        titles.
    widths              tuple: the widths of each column.
    alignments          tuple: alignments for each column:('C', 'L', 'R')
    font_params         nested tuple: containing font name and size
    '''

    REQUIRED_PROPERTIES_KEYS = ('xpos', 'ypos', 'data', 'widths',
                                'alignments', 'font_params' )

    properties: Dict[str, Union[Tuple, List, int]]

    def __init__(self, **properties):
        if type(properties) != dict:
            raise TypeError("Table properties must be a dictionary")
        for a_property_key in Table.REQUIRED_PROPERTIES_KEYS:
            if a_property_key not in properties:
                raise ValueError(f"{a_property_key} has not been supplied to Table constructor")
        self.properties = properties


class TextBlock:
    '''
    xpos                int: postion from left margin in mm.
    ypos                int: postion from top margin in mm.
    lines_of_text       list: for example:
                        ['This is line 1', 'And this line 2', 'The third line etc']
    widths              tuple: the widths of the banner of each line. The banner is a
                        box that surronds each line of text and is important
                        for spacing on the page.
    alignments          tuple: alignments of text within each banner, see Table above
    font_params         nested tuple: containing font name and sizes
    '''
    REQUIRED_PROPERTIES_KEYS = ('xpos', 'ypos', 'width', 'lines_of_text',
                                'alignments', 'font_params' , 'heights')

    def __init__(self, **properties):
        if type(properties) != dict:
            raise TypeError("TextBlock properties must be a dictionary")
        for a_property_key in TextBlock.REQUIRED_PROPERTIES_KEYS:
            if a_property_key not in properties:
                raise ValueError(f"{a_property_key} has not been supplied to TextBlock constructor")
        self.properties = properties


#--------------------------------------------------------
#----------------------- TESTING ------------------------
#--------------------------------------------------------

class TestTextBlockConstructor(unittest.TestCase):

    correct_params_for_this_object = {  'xpos' : 70,
                    'ypos' : 100,
                    'width' : 150,
                    'lines_of_text' : ("line1", 'line2'),
                    'height' : 5,
                    'font_params' : ('DejaVu-Bold', 15),
                    'alignments' : 'C'}

    def test_TextBlock_parameter_not_dict(self):
        with self.assertRaises(TypeError):
            obj = TextBlock(5)


    def test_TextBlock_missing_key_in_parameter(self):
        missing_param = dict(TextBlockConstructorTest.correct_params_for_this_object)
        del missing_param['xpos']
        with self.assertRaises(ValueError):
            obj = TextBlock(**missing_param)


    def test_TextBlock_object_successfully_constructed(self):
        obj = TextBlock(**TextBlockConstructorTest.correct_params_for_this_object)
        self.assertIsInstance(obj, TextBlock)


class TestTableConstructor(unittest.TestCase):

    data = [["Date", "Description", "Job Ref", "Cost"],
            [1, "10.00 hrs worked plus £25.00 travel allowance", "XYZ008", 325.00],
            [2, "8.50 hrs worked plus £25.00 travel allowance ", "XYZ008", 280.00],
            [3, "8.25 hrs worked plus £25.00 travel allowance ", "XYZ008", 272.50],
            [8, "8.00 hours worked only", "XYZ008", 240.00]]

    correct_params_for_this_object = {  'xpos' : 70,
                    'ypos' : 100,
                    'widths' :  (18, 100, 32, 32),
                    'data' : data,
                    'height' : 5,
                    'font_params' : (('DejaVu-Bold', 11), ('DejaVu', 10)),
                    'alignments' : ('C', 'L', 'C', 'R')}


    def test_Table_parameter_not_dict(self):
        with self.assertRaises(TypeError):
            obj = Table(5)


    def test_Table_missing_key_in_parameter(self):
        missing_param = dict(TableConstructorTest.correct_params_for_this_object)
        del missing_param['xpos']
        with self.assertRaises(ValueError):
            obj = Table(**missing_param)


    def test_Table_object_successfully_constructed(self):
        obj = Table(**TableConstructorTest.correct_params_for_this_object)
        self.assertIsInstance(obj, Table)

invoice_to_PDF.py. This 'formats' the Invoice dictionary values into either TextBlock or Tables to be passed to the PDF generator.

2) Am I barking up the wrong tree ? I.e. have I gone about this code totally the wrong way? I was thinking about organising the Invoice dictionary in line order as it appears on the invoice, creating a dict of string constants and then creating another dictionary of rules (about how to combine Invoice & string constants) and finally running all three through a loop, the final output being a combination of Invoice values and string literals combined as per 'the rules' dictionary. This seems almost as complicated as the system I've come up with here...

invoice_to_PDF.py.

import unittest

from generate_PDF import *
from tables_and_text import Table, TextBlock
from helperfunctions import safeget
from invoice import Invoice


# ------------------- TextBlock: 'For work carried out etc.' -------------------

def create_invoice_info_TextBlock(job_consts, date):
    invoice_description = (f"For work carried out at {job_consts['name']}, "
            f"{job_consts['job_address']},",
            f"{job_consts['city']}.",
            f"Month ending {date['month_last_day']} {date['month']} {date['year']}")
    return TextBlock(xpos=int((PDF.PAGE_WIDTH_MM - 150) / 2), ypos=100, width=150,
                        lines_of_text=invoice_description, font_params=(('DejaVu', 15), ('DejaVu', 16), ('DejaVu-Bold', 15)),
                        heights=(12, 6, 10), alignments='C')


# ------------------------------ Table Creation ------------------------------

def calc_cost_from(hours, TA_for_this_day, TA, rate) -> float:
    return hours * rate + (TA * TA_for_this_day)


def format_main_table_data_for_table_class_constructor(data, TA, ref, rate):
    table_text = [["Date", "Description", "Job Ref", "Cost"]]
    for date, daily_job_data in data.items():
        a_row = []
        a_row.append(str(date))
        description_text = f"{daily_job_data.hours:.2f} hrs worked plus"
        if not daily_job_data.TA_for_this_day:
            description_text = f"{daily_job_data.hours:.2f} hours worked only"
        else:
            description_text = description_text + f" £{TA:.2f} travel allowance"
        a_row.append(description_text)
        a_row.append(ref)
        cost = calc_cost_from(daily_job_data.hours, daily_job_data.TA_for_this_day, TA, rate)
        a_row.append(f"{cost:.2f}")
        table_text.append(a_row)
    return table_text


def create_main_table(invoice, rate_of_pay):
    idata = safeget(invoice, 'Data')
    ta = safeget(invoice, 'Job_Constants', 'travel_allowance')
    ref = safeget(invoice, 'Job_Constants', 'ref')
    main_table_data = format_main_table_data_for_table_class_constructor(idata, ta, ref, rate_of_pay)
    table_widths = (18, 100, 32, 32)
    font_properties_main = (('DejaVu-Bold', 11), ('DejaVu', 10))
    return Table(ypos=150, xpos=int((PDF.PAGE_WIDTH_MM - sum(table_widths)) / 2) ,
                        data=main_table_data, widths=table_widths,
                        alignments=('C', 'L', 'C', 'R'), font_params=font_properties_main)


def create_pdf(invoice):
    rate_of_pay = 30.00

    pdf = PDF(orientation = 'P', unit = 'mm', format = 'A4')
    pdf.add_font('DejaVu', '', '/usr/share/fonts/truetype/dejavu/DejaVuSans.ttf', uni=True)
    pdf.add_font('DejaVu-Bold', '', '/usr/share/fonts/truetype/dejavu/DejaVuSans-Bold.ttf', uni=True)
    pdf.add_page()

    invoice_info = create_invoice_info_TextBlock(safeget(invoice, 'Job_Constants'),
                                                    safeget(invoice, 'Date'))
    main_table = create_main_table(invoice, rate_of_pay)

    pdf.apply_text_objects(invoice_info, main_table)
    return pdf

def save_pdf(pdf, filename):
    pdf.output(filename,'F')
    print("---------------------- PDF file saved ! ----------------------")

#--------------------------------------------------------
#----------------------- TESTING ------------------------
#--------------------------------------------------------
# python3 -m unittest -v invoice_to_PDF

class TestTextBlockCreationFunctions(unittest.TestCase):

    Invoice = {
        'Job_Constants': {
            'name' : 'Unit 6',
            'job_address' : 'East Winsley Street',
            'city' : 'Branburn',
            'ref' : 'XYZ 008',
            'travel_allowance' : 25.00
        },
        'Date' : {
            'year' : 2023,
            'month_last_day' : 6,
            'month' : 'June'
        }
    }

    def test_create_invoice_info_TextBlock_return_type(self):
        result = create_invoice_info_TextBlock(safeget(TestTextBlockCreationFunctions.Invoice, 'Job_Constants'),
                                                safeget(TestTextBlockCreationFunctions.Invoice, 'Date'))
        self.assertIsInstance(result, TextBlock)
        self.assertIsInstance(result.properties, dict)


class TestTableCreationFunctions(unittest.TestCase):

    from dataclasses import dataclass

    @dataclass
    class DailyJobData:
        hours: float = 0.0
        TA_for_this_day: bool = True

    Invoice = {
        'Job_Constants': {
            'name' : 'Unit 6',
            'job_address' : 'East Winsley Street',
            'city' : 'Branburn',
            'ref' : 'XYZ 008',
            'travel_allowance' : 25.00
        },
        'Date' : {
            'year' : 2023,
            'month_last_day' : 6,
            'month' : 'June'
        },
        'Data' : {
            1 : DailyJobData(10),
            8 : DailyJobData(8, False),
            2 : DailyJobData(8.5)
        }
    }

    test_data_for_table = [['Date', 'Description', 'Job Ref', 'Cost'],
             ['1', '10.00 hrs worked plus £25.00 travel allowance', 'XYZ 008', '325.00'],
             ['8', '8.00 hours worked only', 'XYZ 008', '240.00'],
             ['2', '8.50 hrs worked plus £25.00 travel allowance', 'XYZ 008', '280.00']]


    def test_calc_cost_from(self):
        result1 = calc_cost_from(10, False, 25, 10)
        result2 = calc_cost_from(10, True, 25, 10)
        self.assertEqual(result1, 100)
        self.assertEqual(result2, 125)


    def test_format_main_table_data_for_table_class_constructor(self):
        idata = safeget(TestTableCreationFunctions.Invoice, 'Data')
        ta = safeget(TestTableCreationFunctions.Invoice, 'Job_Constants', 'travel_allowance')
        ref = safeget(TestTableCreationFunctions.Invoice, 'Job_Constants', 'ref')
        main_table_data = format_main_table_data_for_table_class_constructor(idata, ta, ref, 30)
        self.assertEqual(main_table_data, TestTableCreationFunctions.test_data_for_table)


    def test_create_main_table_return_type(self):
        TABLE_REQUIRED_PROPERTIES_KEYS = ('xpos', 'ypos', 'data', 'widths',
                                'alignments', 'font_params' )
        result = create_main_table(TestTableCreationFunctions.Invoice, 30)
        self.assertIsInstance(result, Table)
        self.assertIsInstance(result.properties, dict)
        for a_key in TABLE_REQUIRED_PROPERTIES_KEYS:
            self.assertIn(a_key, result.properties)
        self.assertEqual(result.properties['alignments'], ('C', 'L', 'C', 'R'))
        self.assertEqual(result.properties['widths'], (18, 100, 32, 32))

if __name__ == '__main__':
    FILENAME = "~/Invoice2PDF/Test_Invoice.pdf"
    save_pdf(create_pdf(Invoice), FILENAME)

generate_PDF.py (I've not included any tests as despite my best efforts to slim the code down, it's still a lot of code !)

from fpdf import *
from helperfunctions import safeget, always_get_item
from tables_and_text import Table, TextBlock

class PDF(FPDF):

    PAGE_WIDTH_MM = 210

# REQUIRED_PROPERTIES_KEYS = ('xpos', 'ypos', 'data', 'widths', 'alignments', 'font_params' )
    def __create_table(self, properties):
        '''
        __Table is applied to PDF one row at a time, one cell at a time. The position of the
        next cell is dictated by cell_start, (see comment later).
        '''
        self.set_y(properties['ypos'])
        self.set_x(properties['xpos'])

        # if there are 2 columns cell_start should be [0, 1], 5 columns [0, 0, 0, 0, 1]
        # 1 tells cell_start to move to the position of the first column, 0 just to the end
        # of that column.
        l_td = len(properties['data'][0])
        cell_start = [0 if i != l_td - 1 else 1 for i in range(l_td) ]

        for idx, row in enumerate(properties['data']):
            self.x = properties['xpos']
            temp_properties_tuple = always_get_item(properties['font_params'], idx)
            self.set_font(temp_properties_tuple[0], size=temp_properties_tuple[1])

            for table_idx, element in enumerate(row):
                    self.cell(w = always_get_item(properties['widths'], table_idx), h = 5,
                                align = always_get_item(properties['alignments'], table_idx),
                                txt = element, border = 1,
                                ln = cell_start[table_idx])


    def __create_text_block(self, properties):
        '''
        properties should contain these keys: 'xpos', 'ypos', 'width', 'lines_of_text',
        'alignments', 'font_params' which are supplied as parameters from the object passed to
        the calling function. The checking of the properties is done elsewhere.
        This function applies 1 or more lines of text to the pdf in accordance with the properties
        supplied.
        '''
        self.set_xy(properties['xpos'], properties['ypos'])

        for idx, text in enumerate(properties['lines_of_text']):

            temp_properties_tuple = always_get_item(properties['font_params'], idx)
            self.set_font(temp_properties_tuple[0], size=temp_properties_tuple[1])

            self.cell(w = properties['width'], h = always_get_item(properties['heights'], idx),
                        align = always_get_item(properties['alignments'], idx), txt = text,
                        border = 0, ln = 1)

            self.set_x(properties['xpos'])


    def apply_text_objects(self, *user_PDF_objects):
        for PDF_obj_type in user_PDF_objects:
            tt = type(PDF_obj_type).__name__
            if tt == 'Table':
                self.__create_table(PDF_obj_type.properties)
            elif tt == 'TextBlock':
                self.__create_text_block(PDF_obj_type.properties)
            else:
                raise TypeError('Objects passed to this function should be of type Table or TextBlock')

And finally helperfunction.py

# From https://stackoverflow.com/questions/25833613/safe-method-to-get-value-of-nested-dictionary
def safeget(dct, *keys):
    for a_key in keys:
        try:
            dct = dct[a_key]
        except KeyError:
            return None
    return dct

In Summary:

0) How are my unit tests ?

1) Should class Table and TextBlock be dataclasses here ?

2) Am I barking up the wrong tree ? I.e. have I gone about this code totally the wrong way? Although I understand my code, do you ?

3) Anything else that stands out much appreciated...

This post is also an invoice to PDF generator, but it's content is all static, I believe mine is much more generalisable and content will potentially be able to be modified without a class re-write, i.e. PDF generation and the invoice are reasonably decoupled (and complex unfortunately). I've also taken on board some of the points made in that post.

Answer 1

bug tracker

Yay, shopworn code! You have been maintaining this for at least two months, we can see. I imagine that during that time bug reports and feature requests have cropped up.

By now, you are an expert on the Pain Points of this codebase. When tracking down bugs, was it easy? Or was there some aspect that made it difficult? Obscure deeply nested datastructures sometimes make it difficult, but as straightforward an issue as "it takes one minute to startup" can also impact bug diagnosis. So taking "quick startup of complex module" can be a feature request that gets prioritized, to help with bug fixes.

Review your bug tracking DB, and look for trends in "difficult" diagnoses. Review git log --patch or your PRs, and look for cases where there was "too much" rework of existing code when adding on a feature request.

The presence of a test suite is certainly encouraging; it indicates this project has acquired some maturity and is in a good position to roll with the punches when new issues crop up. The $ python -m unittest runner is a good one, which I routinely use, but I also use pytest for things it's good at. Consider kicking your testing up to the next level with coverage measurement:

$ pytest --cov --cov-report=term-missing .

You don't need to hit 90% coverage or any other arbitrary metric. But seeing which recently added modules are uncovered may help you decide which test you'd like to write next. The process is cyclic, just like your edit-run-debug cycle. (Is all my code covered by tests? No. But I have good information to help me assess my current test suite. And it is easily gathered.)

---

lowercase

    TA_for_this_day: bool = True

Pep-8 requests spellings like lowercase ta and pdf. Ok, enough on that topic through end of review.

I'm sure folks at your shop know exactly what "TA" denotes, and it probably isn't Teaching Assistant. I confess I don't know what it means. There's no comment. You don't need to write one, but mysteries such as this can make it harder to hire effective maintenance engineers down the road.

EDIT: From subsequent context much further down, this apparently describes a Travel Allowance.

Also, please enclose comments such as "Due to the reduced nature..." within # comment characters, rather than putting them in a string literal that we compute and then discard. Creating """docstrings""" in the proper place is very useful. Don't try to use throwaway string literals for similar purpose.

---

unambiguous representations

        'telephone' : '07889 578685',

Most of your clients live in the UK, I get it. Consider throwing a +44 country code in there anyway, just in case. You can even have display routines strip such prefix upon output if there's some aesthetic goal to be served. Data representations tend to live for years longer than the various programs that manipulate them, so it's worth getting them right.

When my US colleagues code up a "ten-digit telno" like 617-555-1212, I often will try to get them to prepend +1 , or at least a 1- prefix.

Sooner or later someone will make a typo and elide a digit. Having all numbers conform to a single written spec makes it easier to write audit reports that help us investigate and improve data quality issues.

For similar reasons, none of my code uses "23" to represent the current year.

        'month' : 'June'

Well, there's no ambiguity here. But consider using 6 to represent this concept, and then output routines can format it as needed. I guess the theme is, these parsed out fields seem to be less about core concepts and more focused on display details than I would have anticipated.

Also, maybe "Constants" is "Customer Constants"? Or better, "Customer attributes"? I mean, I bet one or two long-term customers have changed their phone number over the years.

---

I'm using FPDF1 ...

Consider using FPDF2, at least for new functionality. I've never used it, but the marketing material seems to touch on at least one of your pain points.

---

separate test code from target code

This looks almost like a #! shebang, but it isn't.

# python3 -m unittest -v tables_and_text

class TestTextBlockConstructor(unittest.TestCase):

Putting TestTextBlockConstructor in the same module clearly works. But it's weird. Recommend you break it out into a module that has "test" in its name.

There's at least two motivations for the separation.

1. It should be straightforward to package up target code, for a production deployment, which contains zero lines of test code.
2. Unit tests should "come in through the front door", accessing target code like any other caller of your pip package or library.

Sometimes people get tripped up on that 2nd one by using odd import spellings, or assuming some global constant is already within scope. Separation leads to better code hygiene.

---

    def test_TextBlock_parameter_not_dict(self):
        with self.assertRaises(TypeError):
            obj = TextBlock(5)

Excellent naming. The "not dict" tells me exactly what behaviour we're trying to test.

    def test_TextBlock_object_successfully_constructed(self):
        obj = TextBlock(**TextBlockConstructorTest.correct_params_for_this_object)
        self.assertIsInstance(obj, TextBlock)

Meh. Consider deleting that final line. We evaluate the ctor for side effects -- we verify it doesn't blow up. The final line is a bit vacuous -- of course we get one of those back. But IDK, maybe you find greater documentation value in that final line than I do.

Here's what I would like to see. Rename obj to block or something. And then, more importantly, exercise its behavior. Maybe demonstrate that the last lines_of_text has length 5 or something.

Ok, now I scrolled up to read the target code. Right, not much behavior there, I see, that's fine.

        if type(properties) != dict:

Prefer if not isinstance(properties, dict):, so that OrderedDict and other subclasses work. (You're not going with Duck Typing, and I guess that's ok, since your goal here is "shallow bugs", immediately telling caller exactly how they messed up if they accidentally pass in a str or something.)

        for a_property_key in TextBlock.REQUIRED_PROPERTIES_KEYS:

Wow, that's verbose! A simple for key in ... would suffice, or even for k in .... Items with broader scope, such as REQUIRED_PROPERTIES_KEYS, have a greater documentation burden to bear, and that particular name is excellent. But with ephemeral local variable the context is right there and often a shorter name will be a better aid to human comprehension.

Also, prefer ... in self.REQUIRED_PROPERTIES_KEYS:

---

modern interpreter

You didn't mention the execution environment. I'm guessing it isn't cPython 3.11.

from typing import Dict, Union, List, Tuple

With recent interpreters it's preferrable to use lowercase dict, list, tuple in type hints.

---

type hints

class TextBlock:
    ...
    def __init__(self, **properties):
            ...
            if a_property_key not in properties:

Ok, we're verifying that some keys are present, that's good, that's helpful, it might suffice. It doesn't go quite as far as attrs, in that we can't say much about whether each key maps to None or an int or whatever. You don't have to make this ctor more bullet proof. But if you've seen buggy callers try to sneak junk past this test, you might consider beefing it up.

        self.properties = properties

I bet this is fine, no need to change it. But I will just note in passing there's a shared reference to caller's underlying object. If he e.g. does del my_prop['xpos'], that will affect self.properties, as well. So, there's absolutely no need, but you might consider storing a copy of the dict that was passed in, depending on your paranoia level.

The docstring is very nice. If I had a problem I would consult it first and pay attention to what it tells me. Frankly, I tend to trust code more than comments or docs, as sometimes things change and docs don't always keep up.

@dataclass
class Table:

Yay, dataclass!

    properties: Dict[str, Union[Tuple, List, int]]

Uggh, terrible. It is a dataclass in name only.

I guess we know something about those dict values, now, and mypy can verify. But it's pretty weak, it won't quash a bunch of bugs in practice. And it's hard to know what well-formed tuples and lists would contain.

Prefer to break out xpos and the others. For one thing, then you can type hint xpos: int or float or whatever. And you can tell us lines_of_text: list[str]

---

class TestTableConstructor(unittest.TestCase):

    data = [["Date", ...
            [8, ...

Well, I guess I'm a little surprised that eight represents a date, but ok, to each his own.

Both text block and table seem to have a combination of "content" and "display presentation" attributes. Consider factoring that last one out into a small class of its own.

---

test what matters

The text block and table tests are mostly copy-n-paste. Now, in test code, copy pasta can be perfectly OK -- it's the one place where we embrace it, to make tests readable and simple with no hidden moving parts. But honestly, I'm not sure what those tests are buying us, other than exercising the ctors which certainly is a good thing.

We spend time writing tests, in order to save time.

What time wasting mistakes are you worried about, which would be time consuming to debug? Caller omitted a mandatory field? Ok, check, got that one covered. Caller accidentally switched a text block for a table? Dunno if that happens much, but it would be an easy test to write and we didn't attempt it.

A dataclass is sooo simple and self explanatory that maybe it doesn't need a test at all. OTOH, it's likely you have a method or two which slurps up some input and creates a text block or table. Now that would be well worth testing for expected behavior, and I didn't see anything of that complexity so far.

Code is risky -- sometimes a line of code blows up. Write tests to mitigate risk, to thoroughly exercise target code and put it through its paces, verifying it does what the spec asked of it.

If a certain class, like Table, has never produced a bug / error report, then maybe it's perfect and it doesn't need any test coverage. Perhaps it is so simple that it can't fail. It's also possible that it's unused so no one cares how it behaves.

I see no tests for format_main_table_data_for_table_class_constructor, for example. It has conditional logic so there's more than one visible behavior a test could reveal.

---

explicit import

from generate_PDF import *

No, please don't do that.

It's easy for the machine to deal with that. But you're being unkind to the Gentle Reader.

Comment the line and let Intelli/J PyCharm or other IDE show you which symbols were coming from there. Or just let isort tidy it up for you.

---

ASCII art

# ------------------------------ Table Creation ------------------------------

You like ASCII art. Ok, that makes one of us, c'est la vie.

Consider leveraging python's """docstring""" facility.

Or break out a separate table_creation.py module if you feel things should be grouped together. Whatever.

---

    return hours * rate + (TA * TA_for_this_day)

nit: That's oddly phrased. Either elide the extra ( ) parens, or use them on both terms if you feel algebraic order of operations needs some emphasis here.

        description_text = f"{daily_job_data.hours:.2f} hrs worked plus"
        if not daily_job_data.TA_for_this_day:
            description_text = f"{daily_job_data.hours:.2f} hours worked only"
        else:
            description_text = description_text + f" £{TA:.2f} travel allowance"

That seems harder to read than necessary. Please elide the not, so the if / else clauses are flipped. Oh, wait, now that I'm reading it that way, it's crazy to have three assignments to that variable. Delete the initial one, please, leaving us with one each for the if / else clauses. Use extra ( ) parens if you find that a source code line gets long enough to split to two lines.

---

magic number

I think 150 describes paragraph width in millimeters. It would be worth inventing a name for its repeated appearances.

---

imports at top

class TestTableCreationFunctions(unittest.TestCase):

    from dataclasses import dataclass

    @dataclass
    class DailyJobData:
        hours: float = 0.0
        TA_for_this_day: bool = True

Promote the import so it goes where it belongs at top-of-file, please. But, why is test code declaring DailyJobData at all? If it's an important concept, shouldn't the target code be taking care of it?

---

simple tests

    def test_calc_cost_from(self):
        result1 = calc_cost_from(10, False, 25, 10)
        result2 = calc_cost_from(10, True, 25, 10)
        self.assertEqual(result1, 100)
        self.assertEqual(result2, 125)

The temp vars are tedious and unhelpful. Prefer a pair of one-liners:

        self.assertEqual(100, calc_cost_from(10, False, 25, 10))
        self.assertEqual(125, calc_cost_from(10, True, 25, 10))

---

use pathlib

    FILENAME = "~/Invoice2PDF/Test_Invoice.pdf"

That's kind of weird, in that I would expect to be able to pass a filename to open(), and I would expect that one to fail. Prefer:

    FILENAME = Path("~/Invoice2PDF/Test_Invoice.pdf").expanduser()

Ok, I'm going to stop reading there, my eyes are glazing over.

---

Overall, I see a codebase whose author is clearly focused on quality.

Every time you add a feature, it's a sure thing you're going to have to interactively run that new code a couple of times to make sure it's working properly.

I don't get the sense you're reaping as much benefit from unit tests as you could be. Try this. Next time you add a feature, don't do anything to run that new code in the usual way. Instead, write a test method which exercises it. It won't work properly the first time, things never do, that's fine. Go through the edit debug cycle by revising the test and target code, until you're happy with it. Then run it the usual way and be happy with the result. Plus, there's a new test in the codebase forever, looking for next month's regressions. And you got it for free!