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

Two (possible) improvements at dataclasses.rst #91687

Open
xandrade opened this issue Apr 19, 2022 · 4 comments
Open

Two (possible) improvements at dataclasses.rst #91687

xandrade opened this issue Apr 19, 2022 · 4 comments
Labels
docs Documentation in the Doc dir easy

Comments

@xandrade
Copy link
Contributor

xandrade commented Apr 19, 2022

Documentation

Observed two possible improvements at dataclasses.rst

  1. 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
  2. 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.

@xandrade xandrade added the docs Documentation in the Doc dir label Apr 19, 2022
@AlexWaygood
Copy link
Member

AlexWaygood commented 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.

@ericvsmith
Copy link
Member

ericvsmith commented 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
Copy link
Member

AlexWaygood commented 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
Copy link
Member

ericvsmith commented 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.

Labels
docs Documentation in the Doc dir easy
Projects
None yet
Development

No branches or pull requests

4 participants