Skip to content

bpo-43558: Add base class initialization to dataclass doc#25967

Merged
ericvsmith merged 5 commits into
python:mainfrom
dhoekstra2000:bpo-43558-fix
May 10, 2021
Merged

bpo-43558: Add base class initialization to dataclass doc#25967
ericvsmith merged 5 commits into
python:mainfrom
dhoekstra2000:bpo-43558-fix

Conversation

@dhoekstra2000

@dhoekstra2000 dhoekstra2000 commented May 7, 2021

Copy link
Copy Markdown

This adds an sentence explaining that super().__init__ ought
to be called in the __post_init__ function of dataclasses, and
it adds a code example, since I felt it was clearer than a
textual describtion.

https://bugs.python.org/issue43558

This adds an sentence explaining that `super().__init__` ought
to be called in the `__post_init__` function of dataclasses, and
it adds a code example, since I felt it was clearer than a
textual describtion.
@dhoekstra2000
dhoekstra2000 requested a review from ericvsmith as a code owner May 7, 2021 12:22
@bedevere-bot bedevere-bot added the docs Documentation in the Doc dir label May 7, 2021
Comment thread Doc/library/dataclasses.rst Outdated
def __post_init__(self):
self.c = self.a + self.b

If the dataclass inherits from another class, which defines an :meth:`__init__`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd like to start with something like: "The __init__ function generated by @dataclass does not call base class __init__ methods. If base class __init__ methods need to be called, it's common to do so in a __post_init__ method."

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I have adapted this text slightly in my last commit to reflect that the __init__ ought to be called if the base class has a non-default __init__ method, i.e. the one defined on object (this is also mentioned here in the docs). I was wondering if it should be mentioned that, when dealing with dataclasses, this is likely the case (I think that the generation of __init__ by @dataclass might obscure the presence of a non-default __init__).


def __post_init__(self):
super().__init__(self.side, self.side)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's a great example.

@bedevere-bot

Copy link
Copy Markdown

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.

@dhoekstra2000

Copy link
Copy Markdown
Author

I have made the requested changes; please review again

@bedevere-bot

Copy link
Copy Markdown

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 May 7, 2021 14:29
Comment thread Doc/library/dataclasses.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know that anyone would know what "non-default __init__ method" means. I'd just leave it as "an __init__ method that needs to be called", or similar. Or maybe add words that say that in general, dataclass-generated __init__ methods to not need to be explicitly called, because the derived class __init__ takes care of initializing all fields.

@dhoekstra2000

Copy link
Copy Markdown
Author

I have made the requested changes; please review again

@bedevere-bot

Copy link
Copy Markdown

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 May 8, 2021 15:20
Comment thread Doc/library/dataclasses.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good. How about instead of "the derived class", use "the derived dataclass"? I'm trying to draw the distinction that if a non-dataclass is derived from a dataclass, then the base class's __init__ should in fact be called.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

If I understand PEP 557 correctly, the __init__ method needs to be called if a dataclass inherits from a non-dataclass or if a non-dataclass inherits from a dataclass. I have tried to reflect this in the new wording.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I understand PEP 557 correctly, the __init__ method needs to be called if a dataclass inherits from a non-dataclass or if a non-dataclass inherits from a dataclass. I have tried to reflect this in the new wording.

If a dataclass inherits from a non-dataclass, then it's probably that the base non-dataclass's __init__ does need to be called. That's what __post_init__ is for, among other uses.

If a non-dataclass inherits from a dataclass, then the non-dataclass most likely does need to call the dataclass's __init__.

It's only the case where a dataclass inherits from a dataclass that the base class's __init__ most likely doesn't need to be called. I say "most likely" because you could construct examples where it does need to be called, if the base dataclass's __post_init__ does some processing. But it's probably too complex to mention that here, I'm just mentioning it here to help you understand the issue. Apologies if I'm not being helpful.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is very helpful! Thank you!

@dhoekstra2000
dhoekstra2000 force-pushed the bpo-43558-fix branch 2 times, most recently from 9cac499 to 004f915 Compare May 8, 2021 22:18
@dhoekstra2000

Copy link
Copy Markdown
Author

I have made the requested changes; please review again

@bedevere-bot

Copy link
Copy Markdown

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 May 8, 2021 22:19

@ericvsmith ericvsmith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for your patience. I think we're close.

You might want to add your name to the Misc/ACKS file.

Comment thread Doc/library/dataclasses.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"not need to" isn't correct. Apologies if that was my error. Maybe simpler language is needed, like "... methods don't need to be called, since ...`.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sorry, English is not my native tongue

Comment thread Doc/library/dataclasses.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I understand PEP 557 correctly, the __init__ method needs to be called if a dataclass inherits from a non-dataclass or if a non-dataclass inherits from a dataclass. I have tried to reflect this in the new wording.

If a dataclass inherits from a non-dataclass, then it's probably that the base non-dataclass's __init__ does need to be called. That's what __post_init__ is for, among other uses.

If a non-dataclass inherits from a dataclass, then the non-dataclass most likely does need to call the dataclass's __init__.

It's only the case where a dataclass inherits from a dataclass that the base class's __init__ most likely doesn't need to be called. I say "most likely" because you could construct examples where it does need to be called, if the base dataclass's __post_init__ does some processing. But it's probably too complex to mention that here, I'm just mentioning it here to help you understand the issue. Apologies if I'm not being helpful.

@bedevere-bot

Copy link
Copy Markdown

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.

@dhoekstra2000

Copy link
Copy Markdown
Author

I have made the requested changes; please review again

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@miss-islington

Copy link
Copy Markdown
Contributor

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

@bedevere-bot

Copy link
Copy Markdown

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

@bedevere-bot

Copy link
Copy Markdown

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 10, 2021
pythonGH-25967)

(cherry picked from commit 2a03172)

Co-authored-by: dhoekstra2000 <douwe.hoekstra2512@gmail.com>
ericvsmith pushed a commit that referenced this pull request May 10, 2021
GH-25967) (GH-26019)

(cherry picked from commit 2a03172)

Co-authored-by: dhoekstra2000 <douwe.hoekstra2512@gmail.com>

Co-authored-by: dhoekstra2000 <douwe.hoekstra2512@gmail.com>
ericvsmith pushed a commit that referenced this pull request May 10, 2021
GH-25967) (GH-26018)

(cherry picked from commit 2a03172)

Co-authored-by: dhoekstra2000 <douwe.hoekstra2512@gmail.com>

Co-authored-by: dhoekstra2000 <douwe.hoekstra2512@gmail.com>
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants