Fix parsing of csv template files #219
Conversation
*Values of csv files are converted by position, instead of content * Updated tests to check for regression * Updated documentation and tests to include multiline text.
fpdf/template.py
Outdated
| handlers = ( | ||
| ("name", str.strip), | ||
| ("type", str.strip), | ||
| ("x1", float), |
There was a problem hiding this comment.
i like this approach. "in the name of deterministic behavior" why strip the fields? but this is a good place to put the actual conversions, and less lines of code than what i was considering when i put the util function in.
There was a problem hiding this comment.
Everything got stripped in the original version, so I just left it that way in order not to change the behaviour. The alternative would be to just use str as a NOP.
fpdf/template.py
Outdated
| s = s.strip() | ||
| if not s: | ||
| raise ValueError('Foreground and Background must be numeric') |
There was a problem hiding this comment.
maybe consider putting a different error message as s is "not empty" here, not "not numeric". (unless i am missing something?)
There was a problem hiding this comment.
I phrased it in this way to inform the user about what's supposed to go there. "Not empty" doesn't give that information.
The documentation could also be more specific about this, but fleshing that out is a project for another time...
restrict decimal seperator replacement to float fields
fpdf/template.py
Outdated
| def varsep_float(s): | ||
| """Convert to float with given decimal seperator""" | ||
| # glad to have nonlocal scoping... | ||
| return float(s.replace(decimal_sep, '.')) |
There was a problem hiding this comment.
I hadn't payed enough attention to decimal seperator replacement at first.
It previously was done on any field where the content didn't start with a ' (single quote).
Those are not at all garanteed to be present in text fields, so we need to restrict replacement to actual float value fields.
Fortunately a nested function sees the surrounding scope where the seperator is defined, which makes this easy.
test/template/test_template.py
Outdated
| @@ -28,6 +28,7 @@ def test_template_nominal_hardcoded(tmp_path): | |||
| "align": "I", | |||
| "text": "logo", | |||
| "priority": 2, | |||
| "multiline": 0, | |||
There was a problem hiding this comment.
Did multiline become mandatory?
There was a problem hiding this comment.
I only changed the tests in this area. Previously multiline was not tested at all, so I added it there.
But if it's not mandatory (obviously, since it passed before), I guess both cases should be tested, so I'll remove some of them agein.
Btw.: Are ther any other fields that are optional? The documentation only mentions a default in passing, but doesn't explicitly say which are or aren't mandatory.
Codecov Report
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 84.88% 85.03% +0.14%
==========================================
Files 16 16
Lines 3527 3542 +15
Branches 710 713 +3
==========================================
+ Hits 2994 3012 +18
+ Misses 360 358 -2
+ Partials 173 172 -1
Continue to review full report at Codecov.
|
|
Thank you for your contribution @gmischler ! I removed |
|
@all-contributors please add @gmischler for code |
|
I've put up a pull request to add @gmischler! 🎉 |
As of issue #215, here's the pull request:
Values of csv files are now converted by position, instead of content, so that no kind of text can cause an error.
I also added a converter to make sure that hex and oct color values are accepted for foreground and background in csv files.
Updated the test data to include an entry with only digits as text, to check for the regression (adequately failed before the code fix).
Updated the documentation and examples to reflect the changes and match the test data again.
While I was at it, I also included the recently improved multiline functionality in the documentation examples and in the tests.
Looks like I forgot to actually remove
util.try_to_type()though. Is that used anywhere else?