Skip to content
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

Merged
merged 16 commits into from Dec 16, 2023
Merged

Conversation

tybug
Copy link
Member

@tybug tybug commented Dec 15, 2023

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.

Comment on lines 196 to 199
# _ATOMIC_TYPES was introduced as an optimization in 3.12's dataclasses.
_ATOMIC_TYPES = frozenset(
{
types.NoneType,
Copy link
Member Author

@tybug tybug Dec 15, 2023

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.

@tybug
Copy link
Member Author

tybug commented Dec 15, 2023

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 dict_factory case (which we don't currently use).

@tybug
Copy link
Member Author

tybug commented Dec 15, 2023

Apologies for the commit turnover here. I'm done making changes now.

Note that with the new guard on to_jsonable, we needed some additional cover tests.

Copy link
Member

@Zac-HD Zac-HD left a 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 🤩

hypothesis-python/src/hypothesis/internal/compat.py Outdated Show resolved Hide resolved
@tybug
Copy link
Member Author

tybug commented Dec 15, 2023

ruff wasn't happy with e9ba14c, so I pushed 0739659. It's probably more readable anyway.

@Zac-HD Zac-HD closed this Dec 16, 2023
auto-merge was automatically disabled December 16, 2023 03:57

Pull request was closed

@Zac-HD Zac-HD reopened this Dec 16, 2023
@tybug
Copy link
Member Author

tybug commented Dec 16, 2023

coverage failure due to a 3.12 only branch; my bad. Let's see if this passes ci.

@Zac-HD Zac-HD merged commit 867e56a into HypothesisWorks:master Dec 16, 2023
47 checks passed
@tybug tybug deleted the json-defaultdict branch December 16, 2023 05:21
@tybug tybug restored the json-defaultdict branch December 16, 2023 05:21
@tybug tybug deleted the json-defaultdict branch December 16, 2023 05:21
@Zac-HD
Copy link
Member

Zac-HD commented Dec 16, 2023

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError when drawing from a strategy that produces dataclasses with a defaultdict attribute
2 participants