GeezerVison is showing github.com — Two (possible) improvements at dataclasses.rst · Issue #91687 · python/cpython
Two (possible) improvements at dataclasses.rst #91687DescriptionDocumentation
Observed two possible improvements at dataclasses.rst
- One of the examples at line 705 is throwing an exception. I would kindly suggest to update
x: List = [] by x: list = field(default_factory=list) to avoid the following exception: ValueError: mutable default <class 'list'> for field x is not allowed: use default_factory
- Remove trailing whitespace in line 745 to avoid exception with automated Azure PR Validation.
@ericvsmith I would be more than happy to raise a PR for above.
Linked PRs
ActivityAlexWaygood commented on Apr 19, 2022 Hi @xandrade,
This section of the docs explains why you should use default_factory for mutable dataclass fields, and why dataclasses sometimes raises an exception if you don't. The point of this specific example is to illustrate the kind of code dataclasses would generate if you didn't use a default_factory, if dataclasses even allowed you to not use a default_factory in this situation. The whole reason why dataclasses raises an exception if you try to use code like this is because it would have unfortunate consequences, and this example is intended to illustrate what those consequences would be, if dataclasses didn't raise an exception in this situation.
The docs indicate that the code in this example does raise an exception with the sentence introducing this example:
Using dataclasses, if this code was valid:
Here's the link to the section of the docs with the example.
You're right that there's a missing import of List from typing in the example (and the easy solution here is just to use list instead of List, as was pointed out to you by @JelleZijlstra and @ericvsmith in the previous issue/PR that you created). But the example definitely shouldn't be using a field with a default_factory, because the example is meant to fail. Reacted by Antonio Andrade ericvsmith commented on Apr 19, 2022 I agree with @AlexWaygood about item 1 being okay as it is. That said, this is twice in two days that someone wanted to change that. What could be done to make it more clear that the example should not work?
For item 2, could you create a PR? The file only has 744 lines as of right now, so I'm not sure what you're proposing. AlexWaygood commented on Apr 19, 2022
this is twice in two days that someone wanted to change that.
I don't think so. I think @xandrade opened one issue about this topic (#91673), closed it, and has now opened another one (this one).
But I agree that we could think about possible clarifications to make it even more explicit. We could do something like what was done in this PR:
ericvsmith commented on Apr 19, 2022 Ah. I didn't notice it was the same person. Thanks.
I'd be okay with a comment # This code raises ValueError. As long as we're doing that, we should change List to list. Reacted by Jelle Zijlstra, Alex Waygood, Gurashish Singh, Antonio Andrade and Raymond Hettinger hauntsaninja commented on Apr 26, 2023
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|