Skip to content

Improve dataclass docstring #94686

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

Merged
merged 2 commits into from
Jul 9, 2022
Merged

Improve dataclass docstring #94686

merged 2 commits into from
Jul 9, 2022

Conversation

TomFryers
Copy link
Contributor

This fixes three trivial issues:

  • The summary line was in the wrong tense.
  • Assuming the underscores are not pronounced, 'an __slots__' should be 'a'. If they should be, then the earlier 'a __hash__' would be wrong.
  • 'method function' is a bit of an odd phrase that seems unnecessary.

@TomFryers TomFryers requested a review from ericvsmith as a code owner July 8, 2022 10:33
@ghost
Copy link

ghost commented Jul 8, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@TomFryers
Copy link
Contributor Author

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

My guess is that this is too small for NEWS, but I'm happy to add it if necessary.

@@ -1193,19 +1193,19 @@ def _add_slots(cls, is_frozen, weakref_slot):
def dataclass(cls=None, /, *, init=True, repr=True, eq=True, order=False,
unsafe_hash=False, frozen=False, match_args=True,
kw_only=False, slots=False, weakref_slot=False):
"""Returns the same class as was passed in, with dunder methods
added based on the fields defined in the class.
"""Return the same class as was passed in, with dunder methods added
Copy link
Member

Choose a reason for hiding this comment

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

It's actually no longer true that the same class is returned (in the case of slots=True). I'm not sure of the exact wording that should be used. Otherwise looks good.

Copy link
Member

Choose a reason for hiding this comment

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

Now that I think about it more, I think we should just drop "Return the same class as was passed in", and say "Add dunder methods based on ...".

@ericvsmith
Copy link
Member

My guess is that this is too small for NEWS, but I'm happy to add it if necessary.

Agreed.

assigned to after instance creation. If match_args is true, the
__match_args__ tuple is added. If kw_only is true, then by default
all fields are keyword-only. If slots is true, a __slots__ attribute
is added.
Copy link
Member

Choose a reason for hiding this comment

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

Here is where it should say that if slots is true, a new class is returned.

@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.

@TomFryers
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from ericvsmith July 9, 2022 10:10
Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

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

Thanks!

@ericvsmith
Copy link
Member

ericvsmith commented Jul 9, 2022

I will commit once @TomFryers has signed the CLA.

edit: Hmm, now it's showing as signed (which it wasn't a minute ago). I'll commit once the tests have passed.

@ericvsmith ericvsmith merged commit a10cf2f into python:main Jul 9, 2022
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry, @TomFryers and @ericvsmith, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker a10cf2f6b3766f9dbbe54bdaacfb3f2ca406ea3d 3.10

@bedevere-bot
Copy link

GH-94716 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 9, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 9, 2022
(cherry picked from commit a10cf2f)

Co-authored-by: Tom Fryers <61272761+TomFryers@users.noreply.github.com>
miss-islington added a commit that referenced this pull request Jul 9, 2022
(cherry picked from commit a10cf2f)

Co-authored-by: Tom Fryers <61272761+TomFryers@users.noreply.github.com>
@ZeroIntensity ZeroIntensity removed the needs backport to 3.10 only security fixes label Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants