Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Convert FontEntry to a data class#20118
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| try: | ||
| self.size = str(float(size)) | ||
| except ValueError: | ||
| self.size = size |
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.
Note this is only called in two places, always withsize='scalable', so there's no point to this conversion.
I would vaguely prefer using make_dataclass (as I did in#18517), possibly together with |
I did so, though I'm not sure it's quite as nice. |
This appears to have actually broken the docs |
The explicit construction is probably a bit more resilient to future changes CPython, but I suspect the type annotation version is more readable. On the other hand, we do not (to my knowledge) have any other type annotations in the code base and given that annotations are still in flux (see the discussion about PEP563) I see the reasoning to keep it that way. |
Strange, it does appear to have a docstring; Sphinx just doesn't appear to collect it. |
No need to split over multiple lines and align equals signs.
OK, I added an explicit entry for it, and that should fix the docs. I guess we could drop that if we switch to the class style. |
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 mean this is fine - I don't quite understand the point though. Why is this better than a normal class? It seems identical, but with an extra song and dance for the docstring?
Well, tbh, it was a more effective looking change when it was written with annotations. But really I want to get rid of more flake8 exceptions. |
When these were added inmatplotlib#20118, we had no type annotations, so it madesense to use the functional form. Now that we do, there's no reason notto use the class form.Also, as `FontEntry` has gained more methods, the functional form looksless clear.
When these were added inmatplotlib#20118, we had no type annotations, so it madesense to use the functional form. Now that we do, there's no reason notto use the class form.Also, as `FontEntry` has gained more methods, the functional form looksless clear.
When these were added inmatplotlib#20118, we had no type annotations, so it madesense to use the functional form. Now that we do, there's no reason notto use the class form.Also, as `FontEntry` has gained more methods, the functional form looksless clear.
PR Summary
With Python 3.7, we now havedata classes. I'm not sure if we want to start moving towards those, so I just converted one small one for discussion purposes.
(Plus a little cleanup to drop a flake8 exception)
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).