Skip to content

Conversation

practicalswift
Copy link
Contributor

Prefer foo not in bar to not foo in bar.

In accordance with Style Guide for Python Code (PEP 8):

@fanquake
Copy link
Member

@practicalswift See the pep-8 discussion in #6156. As well as the (clang-format) discussion in #6839 (similar to these kind of changes).

If your going to do this, please combine all your changes into a single pull request. Then a more general discussion can happen about wether this is something we even want to do. As soon as you start half-enforcing style/formatting, you can just end up with "fix-up" pull-requests ever 2 weeks; while continuously breaking patches etc.

Multiple single commit pull requests that only change formatting/styling/trivial things etc are not preferred, as each one requires review, and generates noise in an already pull-request flooded (with not enough people reviewing) repository.

Contributions are always welcome, but please be mindful of the workload you could be creating for others (especially so close to a feature freeze). If possible, combine batches of smaller, related changes into single pull requests. If needed, a single important change can always be broken out, as already happens with the back-porting we do.

@practicalswift practicalswift changed the title [pep-8] Prefer "foo not in bar" to "not foo in bar" [pep-8] Prefer "foo is None" to "foo == None". Prefer "foo not in bar" to "not foo in bar". Jan 19, 2017
@practicalswift
Copy link
Contributor Author

practicalswift commented Jan 19, 2017

@fanquake PRs now combined as requested.

I totally agree that PEP-8 changes pertaining to whitespace, etc is overshooting.

With regards to PEP-8 compliance my suggestion is to keep it to fixing only really obvious non-Pythonic things such as comparing to None, having unused imports, not being explicit about imports, etc - things that should be non-controversial, unlikely to cause any merge conflicts and unlikely to mess up git blame on a large scale.

Basically what @MarcoFalke outlined in this comment:

I think it makes sense to clean up the python code base in qa/ but please make sure you are not "exceeding the goal". We already had cleanup pulls which changed to a max line length to 80 chars among other things. It turned out the patch was impossible to review, actually changed behavior and would have introduced bugs. I'd rather have "ugly" code that works than some PEP8 code that crashes.

Sounds good? :-)

@maflcko
Copy link
Member

maflcko commented Jan 19, 2017 via email

@practicalswift
Copy link
Contributor Author

@MarcoFalke Good points!

These two changes specifically - avoiding foo == None and not foo in bar - are both of them to be considered in the "bikeshed area" in the context of this project? If so I'll close this PR :-)

@practicalswift
Copy link
Contributor Author

Is this one of interest or should I close it? :-)

@practicalswift practicalswift deleted the test-for-membership branch April 10, 2021 19:29
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants