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

Jump to bottom

Formatted code to closer match PEP8 and removed object inheritance. #22449

Closed
wants to merge 1 commit into from
Closed

Formatted code to closer match PEP8 and removed object inheritance. #22449

wants to merge 1 commit into from

Conversation

mattclarke
Copy link

@mattclarke mattclarke commented Sep 29, 2020

Fixed some formatting where it didn't agree with PEP8.
Also, removed instances of inheriting from object.

@the-knights-who-say-ni
Copy link

the-knights-who-say-ni commented Sep 29, 2020

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@mattclarke

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@tirkarthi
Copy link
Member

tirkarthi commented Oct 15, 2020

PEP8 only changes pollute git history and are not generally merged. There are a lot of places where object is inherited from so I am not sure of removing them all in a module is worthy enough.

@mattclarke
Copy link
Author

mattclarke commented Oct 16, 2020

I can remove them all for all modules if you want? :)

@methane
Copy link
Member

methane commented Oct 21, 2020

I'm sorry, but we don't accept formatting only changes in general.

@methane methane closed this Oct 21, 2020
@merwok
Copy link
Member

merwok commented Oct 21, 2020 •
edited

@mattclarke sorry your first PR was a style change! you’re not the first one to have this idea, we know it comes from a good intention, but the CPython project doesn’t take these kind of aesthetic-only changes

  • modules have been contributed by multiple people over 30 years, so there isn’t one style
  • each change has a cost: reviewer time, compute time on the test machines, size of the repo and transfer, etc
  • a cosmetic change doesn’t really improve or fix anything (and has a small risk to break something)
  • cosmetic changes obscure history and make it harder to find who really changed what line when (devs spend a lot of time looking at history to understand changes in a module and track where bugs come from)

I hope this explains why your PR was not accepted, and that if you want to make a contribution you are welcome! Here’s the guide: https://devguide.python.org/

terryjreedy
terryjreedy reviewed Oct 22, 2020
View changes
Copy link
Member

@terryjreedy terryjreedy left a comment

There are several changes that I think are wrong. We could discuss whether to remove the 2.x 'object' in class statements, if we have not done so before, but that should start with an issue and no PR until substantial agreement from coredevs.

Lib/xml/dom/minidom.py
@@ -103,7 +103,7 @@ def insertBefore(self, newChild, refChild):
newChild.nextSibling = refChild
refChild.previousSibling = newChild
if index:
node = self.childNodes[index-1]
node = self.childNodes[index - 1]
Copy link
Member

@terryjreedy terryjreedy Oct 22, 2020

Choose a reason for hiding this comment

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

Not PEP 8, makes code worse

Lib/xml/dom/minidom.py
__slots__ = ('_name', '_value', 'namespaceURI',
'_prefix', 'childNodes', '_localName', 'ownerDocument',
'ownerElement')
Copy link
Member

@terryjreedy terryjreedy Oct 22, 2020

Choose a reason for hiding this comment

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

Bad wrapping.

Lib/xml/dom/minidom.py
@@ -148,7 +148,7 @@ def replaceChild(self, newChild, oldChild):
newChild.parentNode = self
oldChild.parentNode = None
if (newChild.nodeType in _nodeTypes_with_children
or oldChild.nodeType in _nodeTypes_with_children):
or oldChild.nodeType in _nodeTypes_with_children):
Copy link
Member

@terryjreedy terryjreedy Oct 22, 2020

Choose a reason for hiding this comment

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

Wrong change; see section on newline before/after binary operator.
There are several other similarly wrong changes.

Lib/xml/dom/minidom.py
'_attrsNS',
'nextSibling', 'previousSibling')
Copy link
Member

@terryjreedy terryjreedy Oct 22, 2020

Choose a reason for hiding this comment

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

More bad wrapping.

@mattclarke mattclarke deleted the pep8_minidom branch Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terryjreedy terryjreedy

Assignees
No one assigned
Labels
awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@mattclarke @the-knights-who-say-ni @tirkarthi @methane @merwok @terryjreedy @bedevere-bot