Add typing to AFM parser#30134
Conversation
lib/matplotlib/_afm.py
Outdated
| 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.
I'm uncertain if WX 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. |
ksunden
left a comment
There was a problem hiding this comment.
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.
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.
OK, moved back to those.
There was a problem hiding this comment.
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. |
2896ae3 to
f506d95
Compare
Another option is to do something like def get_fontname(self) -> str:
"""Return the font name, e.g., 'Times-Roman'."""
if 'FontName' not in self._header:
raise LookupError(
'Font name not found in AFM header. '
'This may be due to an incomplete or malformed AFM file.'
)
return self._header['FontName']for all of the accesses. The if statement lets |
Should we discuss the overall direction regarding type hints? There was some discussion on Gitter but since there clearly are different opinions, I don't think we should decide this too hastily. |
Also, check some expected conditions at parse time instead of somewhere during use of the data.
PR summary
Also, check some expected conditions at parse time instead of somewhere during use of the data.
PR checklist