bpo-38431: fix dataclasses.InitVar.__repr__#16700
Conversation
Fix repr method of InitVar to work with typing objects.
|
Bit unclear on which branch this should be based off, should I rebase this off master and then create a backport PR? |
|
Please raise the PR to master. The commit was done in master and then 3.8 branch was created. So it has to be fixed in master and will be backported to 3.8 branch. |
|
okay. |
|
based off wrong branch replaced by #16702 |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Thank you. Mostly LGTM, I left just few nitpicks.
| return f'dataclasses.InitVar[{self.type.__name__}]' | ||
| try: | ||
| type_name = self.type.__name__ | ||
| except AttributeError: |
There was a problem hiding this comment.
Functions have the __name__ attribute. I suggest to use isinstance(self.type, type).
| self.assertEqual(C(10).x, 10) | ||
|
|
||
| def test_initvar_repr(self): | ||
| self.assertEqual(repr(InitVar[str]), 'dataclasses.InitVar[str]') |
There was a problem hiding this comment.
There is already a test for repr() in test_init_var_preserve_type. I suggest to remove it from that test.
|
|
||
| def test_initvar_repr(self): | ||
| self.assertEqual(repr(InitVar[str]), 'dataclasses.InitVar[str]') | ||
| self.assertEqual(repr(InitVar[Optional[str]]), |
There was a problem hiding this comment.
I am not sure that InitVar[Optional[...]] makes sense. The example in the documentation does not use Optional, but the default value is None. I suggest to use different example, e.g. List[int]
There was a problem hiding this comment.
I think optional does make sense, but I can change it if you like.
| @@ -0,0 +1,2 @@ | |||
| Fix ``__repr__`` method for ``dataclasses.InitVar`` to work with typing | |||
| objects. | |||
There was a problem hiding this comment.
Please add "Patch by ...".
There was a problem hiding this comment.
I assume these comments also apply to #16702
I'm not sure what you mean here, could you elaborate?
There was a problem hiding this comment.
Adding "patch by " at end of file for credit since this is added to changelog.
There was a problem hiding this comment.
Sorry, it's "patch by full name." as additional sentence for credit. GitHub removed my content with tags <>
There was a problem hiding this comment.
wait, you mean patched by: <Samuel Colvin>?
Or perhaps you're confused since you're commenting on the wrong PR? should be #16702
There was a problem hiding this comment.
I'm just a bit confused since that format isn't used in any other news entries as far as I can tell.
There was a problem hiding this comment.
I answered here since it had the question initially :) The news entry would be as below for other PR. Sorry for the confusion due to markdown rendering.
Fix ``__repr__`` method for ``dataclasses.InitVar`` to work with typing
objects. Patch by Samuel Colvin.
There was a problem hiding this comment.
okay, that's pretty much what I have in the other PR. Is there still something to change?
There was a problem hiding this comment.
Nothing else, I added the verbatim version since patch by would be a separate sentence and not in past tense as above for most cases. But I will leave it to the core dev who is merging to take care of wording and not pedantic about it. Thanks for the changes.
https://devguide.python.org/committing/#handling-others-code and has an example of the format widely used.
Third, ensure the patch is attributed correctly with the contributor’s name in
Misc/ACKS if they aren’t already there (and didn’t add themselves in their patch)
and by mentioning “Patch by <x>” in the Misc/NEWS.d entry and the check-in
message. If the patch has been heavily modified then “Initial patch by <x>” is
an appropriate alternate wording. GitHub now supports multiple authors in a commit.
Add Co-authored-by: name <name@example.com> at the end of the commit message.
|
@serhiy-storchaka I've updated #16702 (the new PR) with your comments except on the news entry as I didn't understand what you were suggesting. |
5 participants
see https://bugs.python.org/issue38431 and the original bug pydantic/pydantic#886
https://bugs.python.org/issue38431