Skip to content

Replaced some instances of ClassName.method with super().method. #5799

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.

Sign up for GitHub

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

Jump to bottom
Closed
atombrella wants to merge 1 commit into from
Closed

Replaced some instances of ClassName.method with super().method. #5799

atombrella wants to merge 1 commit into from

Conversation

@atombrella
Copy link
Contributor

@atombrella atombrella commented Feb 21, 2018

I replaced this pattern in SSLSocket. It's all over the place in the codebase. I don't know if it merits a single ticket for each area, or just the whole code-base.
@atombrella atombrella requested a review from a team as a code owner February 21, 2018 12:45
serhiy-storchaka
serhiy-storchaka requested changes Mar 27, 2018
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Please open an issue on the bug tracker for discussion. Or even two issues, because different modules have different maintainers, and the changes should be reviewed and merged separately.
@bedevere-bot
Copy link

bedevere-bot commented Mar 27, 2018

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.
@methane
Copy link
Member

methane commented Mar 27, 2018

This PR is linked from bpo-31853.
tiran
tiran requested changes Mar 27, 2018
View reviewed changes
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

Don't recycle an unrelated and already closed ticket
@rhettinger
Copy link
Contributor

rhettinger commented Jul 11, 2018

Atombrella, I think you're proceeding under that false assumption that using super() if always or often better than direct parent class references. Python's super() is complicated (and not very fast). It would be easy to accidentally introduce behavior changes.

Another consideration is that the effects of such changes are hard-to-test because the MRO is controlled by subclasses. That means that in general we cannot know for sure where super() is going to forward it call (because it was designed to support cooperative multiple inheritance).

In general, changes from direct parent class references to super() should only be done if there is a known use case and if the current maintainer of a module agrees to the change.
@atombrella
Copy link
Contributor Author

atombrella commented Jul 11, 2018

OK. Since the ticket has been marked as "not as a bug", I'll close my close my pull request. Thanks for the detailed descriptions.
@atombrella atombrella closed this Jul 11, 2018
@atombrella atombrella deleted the super_calls branch July 11, 2018 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changes

8 participants

@atombrella @bedevere-bot @methane @rhettinger @eric-wieser @tiran @serhiy-storchaka @the-knights-who-say-ni