Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor template.py to create FlexTemplate() #228

Merged
merged 30 commits into from
Oct 2, 2021
Merged

Refactor template.py to create FlexTemplate() #228

merged 30 commits into from
Oct 2, 2021

Conversation

gmischler
Copy link
Collaborator

As announced in issue #216.

I decided to name the new class FlexTemplate(). While TemplateCore() indicates that it is the base class of Template(), that fact is an implementation detail and not really relevant to the user. The current name seems better suited to inform about its intended use.

As promised, the "new" Template() can be used in exactly the same way as before, so we should be fully backwards compatible, except where noted below.

The previous code had some unnecessarily complicated data management, first collecting the input text for all pages, and only in the end rendering them all in one go. Now each page gets rendered right before switching to the next one.

I fixed the csv parsing once more (several times, actually), so that most items are now optional, and documented which are.

I discovered that there was an undocumented template field "rotate", which seemed to work in elements dicts. I added that to the csv parser, and to the documentation. There seem to be some issues with the rotating functionality in fpdf though (see issue #226), so unless that can get fixed, maybe it should be labelled experimental.

Added arguments to FlexTemplate.render() so templates can be placed with offset and rotation (same caveat as above with the latter).

There was previously no test for filling more than one page with Template(), so I added one, as well as a test for creating a multipage document, tests about what happens with bad template data, and tests for FlexTemplate().

In my previous PR, I had added default values for missing fields right when loading a csv file. This was kind of silly, because it globally overrides the individual type defaults already in place. Now we just leave the optional values away if they are not supplied, and let the type handlers worry about defaults. Of course, there are special cases...

Incompatible changes:

Changed the handling of code39(), so that it uses the standard template fields instead of x/y/w/h (see issue #227).

Open questions:

The two barcode types have a different bar default width.

  • "barcode": 1 mm
  • "code 39": 1.5 mm

Is there a technical (graphical) reason for that, or did it just accidentally happen?
If the latter, then it should probably be fixed to the same value.
Oh, and are those actually mm, or maybe points?

When a formally incorrect template is used, that may raise a variety of different exceptions, sometimes during loading, and sometimes only on rendering. Maybe some more input checking and error standardisation could be done there.

@codecov
Copy link

codecov bot commented Sep 26, 2021

Codecov Report

Merging #228 (2b2d82e) into master (5b418f1) will increase coverage by 0.53%.
The diff coverage is 87.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
+ Coverage   85.38%   85.91%   +0.53%     
==========================================
  Files          16       16              
  Lines        3557     3656      +99     
  Branches      720      754      +34     
==========================================
+ Hits         3037     3141     +104     
+ Misses        350      334      -16     
- Partials      170      181      +11     
Impacted Files Coverage Δ
fpdf/template.py 79.10% <87.36%> (+13.15%) ⬆️
fpdf/fpdf.py 83.32% <100.00%> (+0.02%) ⬆️
fpdf/syntax.py 94.25% <0.00%> (ø)
fpdf/html.py 95.20% <0.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b418f1...2b2d82e. Read the comment docs.

Copy link
Member

@Lucas-C Lucas-C left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good job on this PR!
🎉🥳
Could you please add a mention of the changes performed in the CHANGELOG.md file?

.gitignore Outdated Show resolved Hide resolved
docs/Templates.md Show resolved Hide resolved
docs/Templates.md Outdated Show resolved Hide resolved
docs/Templates.md Outdated Show resolved Hide resolved
docs/Templates.md Outdated Show resolved Hide resolved
fpdf/template.py Show resolved Hide resolved
fpdf/template.py Show resolved Hide resolved
test/template/mycsvfile.csv Show resolved Hide resolved
test/template/test_flextemplate.py Show resolved Hide resolved
test/template/test_template.py Show resolved Hide resolved
@Lucas-C
Copy link
Member

Lucas-C commented Sep 27, 2021

@allcontributors please add @gmischler for doc

@allcontributors
Copy link

@Lucas-C

I've put up a pull request to add @gmischler! 🎉

@gmischler
Copy link
Collaborator Author

Well, if I'm not missing anything serious, this thing should be done now.

  • Rotating templates as well as template elements works in any combination (turned out to be simpler than expected).
  • Splitting text doesn't modify the target document anymore.
  • Templates can be applied as flexibly as reasonably possible.

Now I can focus on why I actually wanted to do this... 😉

@gmischler
Copy link
Collaborator Author

gmischler commented Oct 1, 2021

And finally, as a bonus feature, it was surprisingly simple to add scaling to FlexTemplate.render().

@Lucas-C
Copy link
Member

Lucas-C commented Oct 1, 2021

Awesome! I will try to review this asap.

@Lucas-C Lucas-C self-assigned this Oct 1, 2021
Copy link
Member

@Lucas-C Lucas-C left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, thank you @gmischler!
You wrote excellent quality code,
and provided useful features for fpdf2 users!

@Lucas-C Lucas-C merged commit ba133ef into py-pdf:master Oct 2, 2021
@Lucas-C
Copy link
Member

Lucas-C commented Oct 2, 2021

I merged this PR.

I also added an extra commit to ensure backward compatibility regarding Template.code39:
48e8851

@gmischler
Copy link
Collaborator Author

gmischler commented Oct 2, 2021

Great job, thank you @gmischler! You wrote excellent quality code, and provided useful features for fpdf2 users!

It was my pleasure (and benefit for a different project).

I also added an extra commit to ensure backward compatibility regarding Template.code39:

Yeah, my approach may have been too unforgiving there... 😉
Unfortunately, your compatibility fix doesn't really work, the type checking code in load_elements() needs a few more special cases for that...
I have a working solution, tough, including a test for the warning... Would I need to create a new PR for that?

@Lucas-C
Copy link
Member

Lucas-C commented Oct 2, 2021

That would be great, yes, thank you!

your compatibility fix doesn't really work

Could you be more precise please?
I have made a new release of fpdf2 including this PR code, and I'd like to estimate how much of a problem this is...

@gmischler
Copy link
Collaborator Author

Could you be more precise please? I have made a new release of fpdf2 including this PR code, and I'd like to estimate how much of a problem this is...

Your code never gets executed, because the type checking in load_elements() will throw a KeyError first.

@Lucas-C Lucas-C changed the title Refactor template.py to create FlexTempalate() Refactor template.py to create FlexTemplate() Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants