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

Closed
xandrade opened this issue Apr 19, 2022 · 5 comments
Closed

Two (possible) improvements at dataclasses.rst #91687

xandrade opened this issue Apr 19, 2022 · 5 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.

Linked PRs

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

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

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

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.

@slateny slateny added the easy label Sep 9, 2022
gpshead pushed a commit that referenced this issue Apr 24, 2023
modernize dataclass example typing `list` rather than `List` and comment as to that line being the alluded too error.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 24, 2023
modernize dataclass example typing `list` rather than `List` and comment as to that line being the alluded too error.
(cherry picked from commit 7ef614c)

Co-authored-by: Allan Lago <35788148+alago1@users.noreply.github.com>
carljm added a commit to carljm/cpython that referenced this issue Apr 24, 2023
* main: (53 commits)
  pythongh-102498 Clean up unused variables and imports in the email module  (python#102482)
  pythongh-99184: Bypass instance attribute access in `repr` of `weakref.ref` (python#99244)
  pythongh-99032: datetime docs: Encoding is no longer relevant (python#93365)
  pythongh-94300: Update datetime.strptime documentation (python#95318)
  pythongh-103776: Remove explicit uses of $(SHELL) from Makefile (pythonGH-103778)
  pythongh-87092: fix a few cases of incorrect error handling in compiler (python#103456)
  pythonGH-103727: Avoid advancing tokenizer too far in f-string mode (pythonGH-103775)
  Revert "Add tests for empty range equality (python#103751)" (python#103770)
  pythongh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic (python#94519)
  pythonGH-65022: Fix description of copyreg.pickle function (python#102656)
  pythongh-103323: Get the "Current" Thread State from a Thread-Local Variable (pythongh-103324)
  pythongh-91687: modernize dataclass example typing (python#103773)
  pythongh-103746: Test `types.UnionType` and `Literal` types together (python#103747)
  pythongh-103765: Fix 'Warning: py:class reference target not found: ModuleSpec' (pythonGH-103769)
  pythongh-87452: Improve the Popen.returncode docs
  Removed unnecessary escaping of asterisks (python#103714)
  pythonGH-102973: Slim down Fedora packages in the dev container (python#103283)
  pythongh-103091: Add PyUnstable_Type_AssignVersionTag (python#103095)
  Add tests for empty range equality (python#103751)
  pythongh-103712: Increase the length of the type name in AttributeError messages (python#103713)
  ...
carljm added a commit to carljm/cpython that referenced this issue Apr 24, 2023
* superopt: (82 commits)
  pythongh-101517: fix line number propagation in code generated for except* (python#103550)
  pythongh-103780: Use patch instead of mock in asyncio unix events test (python#103782)
  pythongh-102498 Clean up unused variables and imports in the email module  (python#102482)
  pythongh-99184: Bypass instance attribute access in `repr` of `weakref.ref` (python#99244)
  pythongh-99032: datetime docs: Encoding is no longer relevant (python#93365)
  pythongh-94300: Update datetime.strptime documentation (python#95318)
  pythongh-103776: Remove explicit uses of $(SHELL) from Makefile (pythonGH-103778)
  pythongh-87092: fix a few cases of incorrect error handling in compiler (python#103456)
  pythonGH-103727: Avoid advancing tokenizer too far in f-string mode (pythonGH-103775)
  Revert "Add tests for empty range equality (python#103751)" (python#103770)
  pythongh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic (python#94519)
  pythonGH-65022: Fix description of copyreg.pickle function (python#102656)
  pythongh-103323: Get the "Current" Thread State from a Thread-Local Variable (pythongh-103324)
  pythongh-91687: modernize dataclass example typing (python#103773)
  pythongh-103746: Test `types.UnionType` and `Literal` types together (python#103747)
  pythongh-103765: Fix 'Warning: py:class reference target not found: ModuleSpec' (pythonGH-103769)
  pythongh-87452: Improve the Popen.returncode docs
  Removed unnecessary escaping of asterisks (python#103714)
  pythonGH-102973: Slim down Fedora packages in the dev container (python#103283)
  pythongh-103091: Add PyUnstable_Type_AssignVersionTag (python#103095)
  ...
ambv added a commit that referenced this issue Apr 25, 2023
)

modernize dataclass example typing `list` rather than `List` and comment
as to that line being the alluded too error.
(cherry picked from commit 7ef614c)

Co-authored-by: Allan Lago <35788148+alago1@users.noreply.github.com>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
@hauntsaninja
Copy link
Contributor

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir easy
Projects
None yet
Development

No branches or pull requests

5 participants