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

bpo-46076: Improve documentation for per-attribute docstrings with __slots__ #30109

Merged
merged 6 commits into from
Dec 19, 2021

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Dec 14, 2021

@bedevere-bot bedevere-bot added the docs Documentation in the Doc dir label Dec 14, 2021
@AlexWaygood AlexWaygood added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes type-feature A feature request or enhancement labels Dec 14, 2021
@TeamSpen210
Copy link

The pydoc code currently checks specifically for dicts, so perhaps the docs should specify only those are expected (or that code changed)?

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Dec 15, 2021

The pydoc code currently checks specifically for dicts

Good catch! Whether the pydoc implementation should be changed is arguable, but I think that discussion should happen in another issue — for now, I'll just update this patch to note that only dict is supported.

@AlexWaygood
Copy link
Member Author

@stevendaprano, you've been heavily involved in the recent python-ideas thread on this topic — any thoughts on this PR? 🙂

@AlexWaygood
Copy link
Member Author

One thing I'm unsure of: is there anywhere else that this feature should be noted? I couldn't find a "docstring tutorial" where a mention of this feature would easily fit in.

(If there are any other places where a mention would be good, I think a link to the data model would suffice — I'm not suggesting duplicating documentation of this feature in multiple places.)

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This was always a very useful little feature not so many people know about. 👍

@rhettinger
Copy link
Contributor

rhettinger commented Dec 15, 2021

The style of the reference manual is terse. Please just make a compact one-line entry noting that _slots_ can accept a dictionary and that the inspect module treats the values of that dictionary as docstrings for the slots.

@rhettinger rhettinger self-assigned this Dec 15, 2021
@AlexWaygood AlexWaygood marked this pull request as draft December 15, 2021 23:24
Copy link

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really great improvement!

@AlexWaygood
Copy link
Member Author

The style of the reference manual is terse. Please just make a compact one-line entry noting that slots can accept a dictionary and that the inspect module treats the values of that dictionary as docstrings for the slots.

@rhettinger, I absolutely take your point that this PR, as currently written, is out of keeping with the style of the rest of the data model documentation. However, I do think the example is helpful, and this feature is not currently documented anywhere else.

If I pare this PR down to a single sentence, as you suggest, would you be open to a proposal (in a separate PR) to add a mention of this feature in the Descriptor HowTo? I can't think of anywhere else where an example of this sort would fit well; __slots__ are only described in detail in the data model and the Descriptor HowTo.

@stevendaprano
Copy link
Member

stevendaprano commented Dec 17, 2021 via email

Doc/reference/datamodel.rst Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@rhettinger
Copy link
Contributor

rhettinger commented Dec 19, 2021

Several thoughts:

  • The descriptor how-to is primarily about descriptors. This is unrelated.
  • This large example doesn't belong anywhere in the docs.
  • This is a really minor feature.
  • We use it only once in the standard library and it isn't visible because the attributes are private.
  • IMO it doesn't even warrant a tutorial section – we only give a couple lines to enumerate() or zip().
  • The feature isn't universal. For example there isn't a way to use it with dataclasses.
  • The use of _slots_ isn't universally considered a best practice. A number of core devs frown upon their use and think of them only an optimization used when there are expected to be a lot of instances. _slots_ gets in the way of inspecting instances with vars() and conflicts with @cached_property.
  • The docstring feature for named tuples only warranted a couple of lines.
  • This really isn't even a feature of _slots_. Rather it is a capability that was added to inspect.
  • The docs not there for "promoting" features. A blog post is better medium for that.
  • Please just make the requested changes and make a small note in the reference manual.

@AlexWaygood AlexWaygood marked this pull request as ready for review December 19, 2021 12:52
@AlexWaygood
Copy link
Member Author

Thanks for the feedback, Raymond -- I appreciate you taking the time! I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@rhettinger: please review the changes made to this pull request.

@miss-islington
Copy link
Contributor

Thanks @AlexWaygood for the PR, and @rhettinger for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-30206 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Dec 19, 2021
@rhettinger
Copy link
Contributor

Thanks for the PR.

@bedevere-bot
Copy link

GH-30207 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Dec 19, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 19, 2021
…_slots__` (pythonGH-30109)

(cherry picked from commit aeb9ef4)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 19, 2021
…_slots__` (pythonGH-30109)

(cherry picked from commit aeb9ef4)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
brandtbucher pushed a commit to brandtbucher/cpython that referenced this pull request Dec 27, 2021
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 skip news type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants