-
Notifications
You must be signed in to change notification settings - Fork 589
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Vendor dataclasses.asdict
#3813
Conversation
# _ATOMIC_TYPES was introduced as an optimization in 3.12's dataclasses. | ||
_ATOMIC_TYPES = frozenset( | ||
{ | ||
types.NoneType, |
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 thought I was being clever by backporting this optimization, but e.g. types.NoneType
is new in python3.10, and I don't particularly want to special case that for an optimization that wasn't even present in older versions to begin with. It's probably more effort than it's worth to try and muck about with this. If people want the micro performance improvement, I think it's reasonable to say they should upgrade their python version.
more trouble than it's worth to backport
It turned out to not be too difficult to add a coverage test for this, so I've gone ahead and done so, except for the custom |
Apologies for the commit turnover here. I'm done making changes now. Note that with the new guard on |
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.
Nice! Two small implementation comments, but looks great overall - including the tests 🤩
ruff wasn't happy with e9ba14c, so I pushed 0739659. It's probably more readable anyway. |
coverage failure due to a 3.12 only branch; my bad. Let's see if this passes ci. |
🎉 |
closes #3812.
A pair of related questions: how heavily should this vendored implementation be tested, and how much (if any) of it can be
# pragma: no cover
'd? There are currently a number of uncovered lines in_asdict_inner
.