Introducing classes to track issues

[ftomassetti]

When working on a project using Pylasu I noticed the need for a few supporting classes, that I introduced in that project but which I think it would be useful to have in Pylasu.

I am adding that here, but I a Pylasu newbie, so a review would be welcome.

I am asking a review to @alessiostalla as I got "inspiration" from what he was doing in a project

[alessiostalla]

Looks good to me, @loradd is the maintainer of Pylasu so I'd let him review and merge

[ftomassetti]

Thank you @alessiostalla !

[loradd]

Looks good to me! I left a minor comment 👍

[loradd]

Should this extend WithIssues?

[ftomassetti]

You are right, I am just not sure of how the field definition works with inheritance, but I made the change

[loradd]

I think we should define the issues list as simply being initialised with [], rather than using the default factory. This, to avoid having to repeat the property definition for each sub-dataclass.

[loradd]

FirstStageResult should have an issues: List[Issue] = default_factory(list) property to avoid a dataclass missing field error.

[loradd]

Note that the issues list here would not be required if following the comment concerning WithIssues below

[loradd]

Same as FirstStageResult and every other WithIssues subclass

[loradd]

Note that the issues list here would not be required if following the comment concerning WithIssues below

[loradd]

To avoid having to repeat the property for each subclass, we could change this to ... = [] rather than using the default_factory which is used for dataclasses. Defining this same class as dataclass would instead induce errors with the constructors.

[alessiostalla]

issues = [] doesn't do what one would expect in Java or Kotlin – it's like a static field so all classes instances would potentially share the same (mutable) list.

I agree that field is only used with @dataclass so either WithIssues has to be decorated with @dataclass or it should be defined as follows:

class WithIssues:
    """Many classes have the necessity of tracking issues"""
    issues: List[Issue]

    def __init__(self):
        self.issues = []

[loradd]

I agree, then each subclass should be responsible of explicitly invoking the WithIssues constructor. If defining WithIssues as @dataclass, each field in the subclasses should be defined as optional and explicit names in creating new objects should be used to avoid confusion (issues would be the first constructor parameter).

EDIT: I think we should go for the solution proposed by @alessiostalla - i.e. adding init = False to the issues field in WithIssues

[ftomassetti]

I should learn some Python :)
I made another attempt, by making WithIssues a dataclass. When doing so, if I defined the value (as field(default_factory=list) for example) then the subclasses complain as it was a field with a default value preceeding fields with a non-default value, so I defined the value of issues in subclasses. Does this make sense?
I thought that if WithIssues was not marked as a dataclass then issues would not have been considered a dataclass field in subclasses, but I could well be wrong

[alessiostalla]

I should learn some Python :) I made another attempt, by making WithIssues a dataclass. When doing so, if I defined the value (as field(default_factory=list) for example) then the subclasses complain as it was a field with a default value preceeding fields with a non-default value, so I defined the value of issues in subclasses. Does this make sense? I thought that if WithIssues was not marked as a dataclass then issues would not have been considered a dataclass field in subclasses, but I could well be wrong

IMHO this kinda defeats the point of WithIssues, because it forces you to redeclare the field in subclasses.
To avoid the problem you could mark the issues field as init: False:

    issues: List[Issue] = field(default_factory=list, init=False)

This won't include the field in the generated constructor, and it will make it possible for subclasses to have mandatory fields. If for a certain subclass one wants to include the issues list in its constructor for whatever reason, they can redeclare the field only for that specific subclass.

[ftomassetti]

Thank you @alessiostalla for the suggestion!

[ftomassetti]

Thank you!
Would it be possible to do a release, so I can use these changes on the PDF Association project?