-
-
Notifications
You must be signed in to change notification settings - Fork 652
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 majority of remaining datatype()
s with @dataclass
#8435
Replace majority of remaining datatype()
s with @dataclass
#8435
Conversation
3ec89c5
to
a889c72
Compare
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.
Looking good. Couple of comments.
59d342c
to
ab1ee50
Compare
class NativeArtifact(datatype(['lib_name']), PayloadField): | ||
# NB: We manually implement __hash__(), rather than using unsafe_hash=True, because PayloadField | ||
# will mutate _fingerprint_memo and we must ensure that mutation does not affect the hash. For | ||
# extra safety, we override __setattr__() to ensure that the hash can never be accidentally changed. |
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.
Does this apply to all PayloadField
s? Does every other PayloadField
also have to check if it's frozen? If so, could we at least at TODO here to turn this into a mixin or decorator to avoid error-prone boilerplate?
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.
Good question. This is the only PayloadField
that used datatype()
. The others do a mix of extending tuple
, OrderedSet
, or implementing their own __hash__()
.
I'm going to hold off on the TODO because this is a bigger question for what to do with PayloadField
. For example, should all of the implementing classes become @dataclass
es? Could we rewrite PayloadField
to be immutable so that we can stop worrying about this __setattr__()
concern? I think a GitHub issue could be appropriate to open!
@@ -130,18 +164,14 @@ def transitive(self): | |||
def copy(self, **replacements): |
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.
This is why it's sometimes useful to use things like DatatypeMixin
, because we might not have had to make any of the changes to this file at all except for moving __new__
to __init__
if we had continued to use the DatatypeMixin
interface with .copy()
and ._asdict()
, which abstracts over the specific way data is represented. Would you be open to it if I submitted a followup (or prework!) PR that demonstrates what I'm talking about?
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.
With the below code, we only had to change line 167 from self._asdict()
to dataclasses.asdict(self)
.
All the other changes were simply refactors in realizing that excludes
will already be processed through the __init__
so there was no need to explicitly call JarDependency._prepare_excludes
.
--
With DatatypeMixin
exposing .copy()
and ._asdict()
, these same behaviors are implemented through dataclass.asdict()
and dataclass.replace()
. Preferably we will only have one and only one way to implement these two behaviors. DatatypeMixin
and those two functions have been useful and were a great utility for the codebase. Now that we have an stdlib version (only possible as of Py3.6), I advocate for going with the solution of leveraging the std lib for better integration with tooling, less code for us to maintain, and lower barrier to entry for new contributors.
This can't use dataclass because we need primitives
80478c0
to
ecd226e
Compare
Builds off of #8423.
For
native.py
, we must usetyping.NamedTuple
because those call sites can only use basic Python primitives. This is the same as a normalnamedtuple
, only that it uses a class-based definition with types built-in.