-
Notifications
You must be signed in to change notification settings - Fork 253
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
Allow keep_aspect_ratio flag in templates. #1119
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -84,6 +84,7 @@ def load_elements(self, elements): | |||||
"priority": int, | ||||||
"multiline": (bool, type(None)), | ||||||
"rotate": (int, float), | ||||||
"keep_aspect_ratio": object, # "bool or equivalent", | ||||||
} | ||||||
|
||||||
self.elements = elements | ||||||
|
@@ -190,6 +191,7 @@ def _varsep_float(s, default="0"): | |||||
("priority", int, False), | ||||||
("multiline", self._parse_multiline, False), | ||||||
("rotate", _varsep_float, False), | ||||||
("keep_aspect_ratio", int, False), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, the converter is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing bold/italic/underline to using Of couse, since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed! |
||||||
) | ||||||
self.elements = [] | ||||||
if encoding is None: | ||||||
|
@@ -423,9 +425,19 @@ def _ellipse( | |||||
pdf.set_line_width(size * scale) | ||||||
pdf.ellipse(x1, y1, x2 - x1, y2 - y1, style=style) | ||||||
|
||||||
def _image(self, *_, x1=0, y1=0, x2=0, y2=0, text="", **__): | ||||||
def _image( | ||||||
self, *_, x1=0, y1=0, x2=0, y2=0, text="", keep_aspect_ratio=False, **__ | ||||||
): | ||||||
if text: | ||||||
self.pdf.image(text, x1, y1, w=x2 - x1, h=y2 - y1, link="") | ||||||
self.pdf.image( | ||||||
text, | ||||||
x1, | ||||||
y1, | ||||||
w=x2 - x1, | ||||||
h=y2 - y1, | ||||||
link="", | ||||||
keep_aspect_ratio=keep_aspect_ratio, | ||||||
) | ||||||
|
||||||
def _barcode( | ||||||
self, | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
img;I;0;0;50;25;;;;;;;;image;;;;;1 | ||
box;B;0;0;50;25;;;;;;;;box;;;;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not typing this as
bool
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was mostly just a little confused here with the types, as other fields also seem to be flags but are typed as objects (eg.
bold
&italic
). Seems that "multiline" is also possibly a bool but it is typed as(bool, type(None)
) for some reason.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmischler just a ping for you on this subject, as you have more experience than me with the templating system 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like I added this in #244.
The motivation was to allow other values with a True/False meaning, similar to what most library functions in Python allow. The documentation also specifies "enabled with True or equivalent value".
If there is a better/more explicit way to specify this, I haven't found it yet...
The other reason was that bold/italic/underline can also be specified in in a csv file, which uses ints instead of bools (more precisely: strings representing ints). This reason does not apply to
keep_aspect_ratio
, but the first one still does.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright!
So what do you think is the best there?
int
?(bool, int)
?IMHO we could stick to
bool
even if non-boolean "truthy" values are also valid, this would make the intent of this parameter clearer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use
bool
, then we need an additional conversion (first fromstr
toint
, then tobool
) for CSV files.You'd expect there to be a standard way of expressing "bool or True/False equivalent", wouldn't you?
A quick search has not turned up anything useful for me...
Maybe just using
bool
is really the simplest way, although that is technically a backwards incompatible change.