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

Replace most usages of datatype() with @dataclass #8362

Merged
merged 2 commits into from Oct 1, 2019

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Sep 29, 2019

#8181 demonstrated that it's possible to use @dataclass(frozen=True) as an alternative to datatype(). As argued there, this brings several benefits, including aligning us more to the stdlib and allowing MyPy to understand our code for free.

This PR takes a first pass to convert >75% of datatype usages to @dataclass. It naively does this translation:

  • Does not attempt to add any more type hints than were previously used.
  • Leaves untouched certain datatypes() that inherit normal classes with class variables, which would get overridden
  • Leaves untouched certain datatypes() with custom __new__() that is not easily replicated

@Eric-Arellano Eric-Arellano force-pushed the no-more-datatype branch 2 times, most recently from 6ad0c41 to 760bb6b Compare September 29, 2019 08:14
@benjyw
Copy link
Sponsor Contributor

benjyw commented Sep 29, 2019

This is hot!! But doesn't it require #8181 to be merged?

@Eric-Arellano
Copy link
Contributor Author

But doesn't it require #8181 to be merged?

Nope! Confirmed with @cosmicexplorer that we can merge this before! #8181 adds a check to ensure that we don't forget to set frozen=True + updates our docs. We can land either in whichever order.

Copy link
Sponsor Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Epic, as usual... :)

@Eric-Arellano Eric-Arellano merged commit 25a8c7a into pantsbuild:master Oct 1, 2019
@Eric-Arellano Eric-Arellano deleted the no-more-datatype branch October 1, 2019 00:14
Eric-Arellano added a commit that referenced this pull request Oct 10, 2019
Builds off of #8362, this time tackling trickier instances.

This introduces a new idiom of using `@dataclass(unsafe_hash=True)`. Certain custom `__new__` implementations could not be implemented using either the normal dataclass auto-generated `__init__()` nor through `__post_init__()`. When `frozen=True`, it is impossible to implement your own `__init__()` because you cannot set attribute values. So, we cannot use `frozen=True` but still must have a hash to work with V2. `unsafe_hash=True` does this for us. We use `@frozen_after_init` from #8431 to ensure that we don't accidentally mutate the object after.
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.

None yet

2 participants