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

Minor mistake in dataclasses documentation update #108267

Closed
FrozenBob opened this issue Aug 22, 2023 · 14 comments
Closed

Minor mistake in dataclasses documentation update #108267

FrozenBob opened this issue Aug 22, 2023 · 14 comments
Labels
3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes docs Documentation in the Doc dir

Comments

@FrozenBob
Copy link
Contributor

FrozenBob commented Aug 22, 2023

An update to the dataclasses docs, intended to make magic method names link to the relevant data model documentation, accidentally changed a line that shouldn't have been changed.

The docs used to say

There is a tiny performance penalty when using frozen=True: __init__() cannot use simple assignment to initialize fields, and must use object.__setattr__().

The documentation update accidentally changed object.__setattr__ to just __setattr__ here, so now it reads

There is a tiny performance penalty when using frozen=True: __init__() cannot use simple assignment to initialize fields, and must use __setattr__().

This line was specifically meant to refer to object.__setattr__, the __setattr__ method of the base object class, as simple attribute assignment would hit the frozen dataclass's __setattr__ override.

This part of the documentation should be reverted. I think it should just take a 1-character change, simply removing a tilde.

Linked PRs

@FrozenBob FrozenBob added the docs Documentation in the Doc dir label Aug 22, 2023
@ericvsmith
Copy link
Member

I'm no sphinx expert. Would removing a tilde restore the output text to be object.__setattr__? That's what I think should be displayed.

@hugovk
Copy link
Member

hugovk commented Aug 22, 2023

Yes, removing the tilde gives object.__setattr__():

image

@FrozenBob Thanks for the report, would you like to create a PR to fix this?

@hugovk hugovk added 3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes labels Aug 22, 2023
@ericvsmith
Copy link
Member

Yes, removing the tilde gives object.__setattr__():

How on earth is someone supposed to know that? Seriously: where could I find out more info? I'd like to get better at this.

@AlexWaygood
Copy link
Member

@ericvsmith there's some useful info on the markup in the devguide here (in particular, see the "quick reference" section at the top of the page): https://devguide.python.org/documentation/markup/

@AlexWaygood
Copy link
Member

AlexWaygood commented Aug 22, 2023

If we just remove the tilde, the link will take people to the entry in the data model documentation for the __setattr__ magic method, rather than the docs for object.__setattr__ itself (we have no docs for object.__setattr__ itself). Maybe it would be better in this case to suppress the link entirely?

:meth:`!object.__setattr__`

That will render as object.__setattr__() in the HTML documentation, but won't add a link.

@FrozenBob
Copy link
Contributor Author

Suppressing the link sounds reasonable to me.

@ericvsmith
Copy link
Member

Suppressing the link sounds reasonable to me.

Agreed.

AlexWaygood pushed a commit that referenced this issue Aug 23, 2023
Fixed a sentence in dataclasses.rst

Changed "__setattr__" to "object.__setattr__" in a section that was specifically supposed to refer to the __setattr__ method of the object class. Also suppressed the link to the data model docs for __setattr__, since we're talking about a specific __setattr__ implementation, not __setattr__ methods in general.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Aug 23, 2023
…nGH-108355)

Fixed a sentence in dataclasses.rst

Changed "__setattr__" to "object.__setattr__" in a section that was specifically supposed to refer to the __setattr__ method of the object class. Also suppressed the link to the data model docs for __setattr__, since we're talking about a specific __setattr__ implementation, not __setattr__ methods in general.
(cherry picked from commit 79fdacc)

Co-authored-by: FrozenBob <30644137+FrozenBob@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Aug 23, 2023
…nGH-108355)

Fixed a sentence in dataclasses.rst

Changed "__setattr__" to "object.__setattr__" in a section that was specifically supposed to refer to the __setattr__ method of the object class. Also suppressed the link to the data model docs for __setattr__, since we're talking about a specific __setattr__ implementation, not __setattr__ methods in general.
(cherry picked from commit 79fdacc)

Co-authored-by: FrozenBob <30644137+FrozenBob@users.noreply.github.com>
AlexWaygood pushed a commit that referenced this issue Aug 23, 2023
…08355) (#108357)

gh-108267: Dataclasses docs: Fix object.__setattr__ typo (GH-108355)

Fixed a sentence in dataclasses.rst

Changed "__setattr__" to "object.__setattr__" in a section that was specifically supposed to refer to the __setattr__ method of the object class. Also suppressed the link to the data model docs for __setattr__, since we're talking about a specific __setattr__ implementation, not __setattr__ methods in general.
(cherry picked from commit 79fdacc)

Co-authored-by: FrozenBob <30644137+FrozenBob@users.noreply.github.com>
Yhg1s pushed a commit that referenced this issue Aug 23, 2023
…08355) (#108358)

gh-108267: Dataclasses docs: Fix object.__setattr__ typo (GH-108355)

Fixed a sentence in dataclasses.rst

Changed "__setattr__" to "object.__setattr__" in a section that was specifically supposed to refer to the __setattr__ method of the object class. Also suppressed the link to the data model docs for __setattr__, since we're talking about a specific __setattr__ implementation, not __setattr__ methods in general.
(cherry picked from commit 79fdacc)

Co-authored-by: FrozenBob <30644137+FrozenBob@users.noreply.github.com>
@AlexWaygood
Copy link
Member

Thanks @FrozenBob!

@FrozenBob
Copy link
Contributor Author

This error appears to have been reintroduced during an overhaul of the dataclasses docs' links. Looks like it needs to be re-fixed.

@hauntsaninja
Copy link
Contributor

Thank you for your vigilance!

miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 16, 2024
pythonGH-119082)

(cherry picked from commit 17cba55)

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 16, 2024
pythonGH-119082)

(cherry picked from commit 17cba55)

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@AlexWaygood
Copy link
Member

Thanks @FrozenBob! We've fixed this on main (again!), and automerge has been scheduled for the backport PRs.

AlexWaygood pushed a commit that referenced this issue May 16, 2024
…cs (GH-119082) (#119098)

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
AlexWaygood pushed a commit that referenced this issue May 16, 2024
…cs (GH-119082) (#119097)

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@FrozenBob
Copy link
Contributor Author

I think you might need an extra blank line before the comment or something - it's rendering in the generated documentation.

@AlexWaygood AlexWaygood reopened this May 17, 2024
@FrozenBob
Copy link
Contributor Author

I see a confused emoji. In case it wasn't clear, this is what's showing up in the docs now:

There is a tiny performance penalty when using frozen=True: __init__() cannot use simple assignment to initialize fields, and must use object.__setattr__(). .. Make sure to not remove “object” from “object.__setattr__” in the above markup

The part starting with the ".." looks like it was supposed to be a reStructuredText comment, but it's showing up in the actual documentation. I think this may be because it needs a blank line above it.

@AlexWaygood
Copy link
Member

I understood the problem — sorry for the ambiguous reaction emoji I applied! I suppose I intended to convey that I was frustrated at myself for not checking the docs preview before merging, but sadly there's no reaction emoji for that exact sentiment ;-)

AlexWaygood added a commit to AlexWaygood/cpython that referenced this issue May 20, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 20, 2024
(cherry picked from commit 423bbcb)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 20, 2024
(cherry picked from commit 423bbcb)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
AlexWaygood added a commit that referenced this issue May 20, 2024
gh-108267 Fix another dataclasses docs typo (GH-119277)
(cherry picked from commit 423bbcb)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
AlexWaygood added a commit that referenced this issue May 20, 2024
gh-108267 Fix another dataclasses docs typo (GH-119277)
(cherry picked from commit 423bbcb)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes docs Documentation in the Doc dir
Projects
None yet
Development

No branches or pull requests

5 participants