search

Strumenta / pylasu

Apache License 2.0
24 stars 4 forks source link

Introducing classes to track issues #26

Closed ftomassetti closed 11 months ago

ftomassetti commented 11 months ago

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 commented 11 months ago

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

ftomassetti commented 11 months ago

Thank you @alessiostalla !

ftomassetti commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

Thank you @alessiostalla for the suggestion!

ftomassetti commented 11 months ago

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