Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Use class form of data classes#27415
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
galleries/examples/widgets/menu.py Outdated
| labelcolor: str = 'black' | ||
| bgcolor: str = 'yellow' |
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.
Couldn't these be any matplotlib color spec type?
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.
True, though I'm not sure this example ever would.
| ], | ||
| namespace={ | ||
| '__doc__': """ | ||
| @dataclasses.dataclass(frozen=True) |
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.
Making this frozen breaks JSON decoding ofFontManager. Either you have to leave this unfrozen, or you have to adaptFontManager._json_decode():
matplotlib/lib/matplotlib/font_manager.py
Lines 937 to 938 in379989e
| r=FontEntry.__new__(FontEntry) | |
| r.__dict__.update(o) |
MaybeFontEntry(**o) will do? Or you have to go withobject.__setattr__ similar tohttps://github.com/jsonpickle/jsonpickle/pull/397/files.
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.
We just need to avoid changing the element after creation, which we can do by modifying the input dictionary beforehand.
I'm not sure why we were doing__new__ and__dict__.update instead ofFontEntry(**o), but I've moved to the latter as suggested.
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
When these were added in#20118, we had no type annotations, so it made sense to use the functional form. Now that we do, there's no reason not to use the class form.
Also, as
FontEntryhas gained more methods, the functional form looks less clear.Also, use one in the menu example.
PR checklist