Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add typing to AFM parser#30134
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:text-overhaul
Are you sure you want to change the base?
Add typing to AFM parser#30134
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lib/matplotlib/_afm.py Outdated
@@ -376,7 +394,8 @@ def get_str_bbox_and_descent(self, s): | |||
except KeyError: | |||
name = 'question' | |||
wx, _, bbox = self._metrics_by_name[name] | |||
total_width += wx + self._kern.get((namelast, name), 0) | |||
total_width += wx |
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'm uncertain ifWX
is the only thing that can be a float, or that's a parsing bug.
Also, |
cc@jkseppan as you were going to do similar for |
We should add a note to the dev docs that we are doing in-line type hints for private modules. |
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.
Overall looks good, aside from docstrings on the remade NamedTuples
lib/matplotlib/_afm.py Outdated
width: float | ||
#: The character width (WX). | ||
name: str | ||
#: The character name (N). | ||
bbox: tuple[int, int, int, int] | ||
#: The bbox of the character (B) as a tuple (*llx*, *lly*, *urx*, *ury*). |
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.
This does not include these comments as docstrings, which I think we would still prefer
I think we should be able to do the__doc__
manual lines still for the attributes
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.
OK, moved back to those.
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.
At least Sphinx picks up docstrings from the class body, but they have to be strings and not comments. While PEP 589 mentions that definitions can be "optionally preceded by a docstring" it seems that Sphinx uses the string following the definition.
lib/matplotlib/_afm.py Outdated
name: bytes | ||
#: Name of the part, e.g. 'acute'. | ||
dx: float | ||
#: x-displacement of the part from the origin. | ||
dy: float | ||
#: y-displacement of the part from the origin. |
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.
Same as CharMetrics
Also, check some expected conditions at parse time instead of somewhereduring use of the data.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
jkseppan commentedJun 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Another option is to do something like defget_fontname(self)->str:"""Return the font name, e.g., 'Times-Roman'."""if'FontName'notinself._header:raiseLookupError('Font name not found in AFM header. ''This may be due to an incomplete or malformed AFM file.' )returnself._header['FontName'] for all of the accesses. The if statement lets |
Should we discuss the overall direction regarding type hints? There was some discussionon Gitter but since there clearly are different opinions, I don't think we should decide this too hastily. |
PR summary
Also, check some expected conditions at parse time instead of somewhere during use of the data.
PR checklist