Closed morphonedev closed 3 years ago
@morphonedev This looks really great! I know I haven't used fonts extensively myself, but I'm surprised nobody else had a need to "complain" or "fix" it like this before. This is a great contribution. Thanks!!
I've added some more or less minor comments, and agree with @claudep otherwise. A separate file like fonts.py is recommended as we already have 1542 lines of code in svglib.py. And after this PR maybe we can find a way to refactor it even a bit more.
This pull request introduces 6 alerts when merging 3f2c4cfab7ef247f49a973188928dd24adc09080 into 7cb36a32867580bd3683b00a7458c2dff1a48e6b - view on LGTM.com
new alerts:
This pull request introduces 2 alerts when merging f652d6110aeda847c21a010e72e146e13ee2ba66 into 7cb36a32867580bd3683b00a7458c2dff1a48e6b - view on LGTM.com
new alerts:
I am so sorry i screwed up the squashing and somehow killed the last commit. I had to recommit the changes for the original pull request.
This pull request introduces 2 alerts when merging ec8e06756516da42dccb9022eef8cd3fd342bab0 into 7cb36a32867580bd3683b00a7458c2dff1a48e6b - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging c6d4c6912c53b546eba8c906a71e7d06aa8de517 into 7cb36a32867580bd3683b00a7458c2dff1a48e6b - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging ea180133288d088d350c8a3e033c00b0f3b6343c into 1e94905cfe8c7e57ee52cae2fbbb774c756d1244 - view on LGTM.com
new alerts:
I'm fine with it if you address the unused imports reported by LGTM above, too.
Regarding the unused import alerts. I am not sure how to get around them, if you want to keep backward compatibility. I dont know any way to export the register_font and find_font methods from svglib without importing them beforehand. This is regarding the point : https://github.com/deeplook/svglib/pull/260#discussion_r559039642. If backward compatibility is not needed I can simply remove them and the user has to import them from svglib.fonts.
Re the imports... ok, I see now. You could make some dummy use of the imported symbols to please LGTM, if only by assigning them to some private vars. With a one line comment of saying why this makes sense. Hmm, but maybe that will cause another such comment by LGTM. ;)
Would the # NOQA
comment satisfy LGTM? Those dummy lines are bad looking for me :-(
This pull request introduces 1 alert when merging 1901176b8dfc0d1c12971d1dcd83c82dfd599eb6 into 3bc8ce63b5a3f2eb19429efddc5bbd4eaba1d5c2 - view on LGTM.com
new alerts:
Since the #NOQA unfortunately did not work, how about something like:
# To keep backward compatibility, since those functions where previously part of the svglib module
from .fonts import (register_font as fonts_register_font, find_font as fonts_find_font) # noqa
def register_font(font_name, font_path=None, weight='normal', style='normal', rlgFontName=None):
return fonts_register_font(font_name, font_path, weight, style, rlgFontName)
def find_font(font_name, weight='normal', style='normal'):
return fonts_find_font(font_name, weight, style)
@morphonedev Maybe even better with leading underscores indicating they should not be used?
from .fonts import (
register_font as _fonts_register_font,
find_font as _fonts_find_font
) # noqa
Anything missing for getting this merged?
From my side there are no open points anymore. If I missed something go ahead and please point it out. @deeplook and @claudep thank you for maintaining this great project. As you can propbably tell I am pretty new to this whole github contribution thing. In fact this was my first. So thank you for putting this much effort in to help an newbie improving.
Thanks, I'll make a final round of review before pushing it.
Thanks again, pushed in 83d2ec043e0
As mentioned, this would be a possible solution to handle font-weight , font-style mapping from svg to certain reportlab fonts. Since the main goal is to provide a mapping from svg font-family, font-weight and font-style combinations to reportlab fontnames, I think this is better placed in svglib and not reportlab.