-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
bpo-33805: Improve error message of dataclasses.replace() #7580
Conversation
@ericvsmith PTAL |
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.
Thanks for the PR. See comments.
Lib/dataclasses.py
Outdated
_FIELD_CLASSVAR: 'ClassVar', | ||
_FIELD_INITVAR: 'InitVar' | ||
} | ||
|
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 don't think it's worth adding this mapping and the new __str__
just for this one error message.
Lib/dataclasses.py
Outdated
@@ -1173,7 +1182,11 @@ class C: | |||
continue | |||
|
|||
if f.name not in changes: | |||
changes[f.name] = getattr(obj, f.name) | |||
if f._field_type is _FIELD_INITVAR: | |||
raise ValueError(f"{f._field_type} '{f.name}' " |
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.
For consistency with the rest of the code, please change '{f.name}'
to {f.name!r}
. Also, just hard-code InitVar
in the message, since this message is only applicable with InitVar
s.
Lib/dataclasses.py
Outdated
if f._field_type is _FIELD_INITVAR: | ||
raise ValueError(f"{f._field_type} '{f.name}' " | ||
'must be specified with replace()') | ||
else: |
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.
The else
is not needed.
Lib/test/test_dataclasses.py
Outdated
c = C(1, 10) | ||
with self.assertRaisesRegex(ValueError, r"InitVar 'y' must be " | ||
"specified with replace()"): | ||
replace(c, i=3) |
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.
Please add a test that shows that an InitVar
does work with replace()
, and check that its value is used (probably via a __post_init__
method). Maybe something like:
def __post_init__(self, y):
self.x *= y
or similar.
@@ -0,0 +1 @@ | |||
Update error message of dataclasses.replace() when InitVar is not specified. |
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'd prefer "Improve error message of dataclasses.replace() when an InitVar is not specified".
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@ericvsmith |
Thanks for making the requested changes! @ericvsmith: please review the changes made to this pull request. |
Thanks! Looks good. I'm going to wait a few days before I merge this, because at this late date I don't want to put this in to 3.7.0, I'll wait for 3.7.1. |
Thanks @corona10 for the PR, and @ericvsmith for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
GH-7876 is a backport of this pull request to the 3.7 branch. |
) (cherry picked from commit 3d70f7a) Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
https://bugs.python.org/issue33805