-
Notifications
You must be signed in to change notification settings - Fork 212
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
PROTON-2322/PROTON-2813: [Python] Finish PEP8 sanitization #423
Conversation
This is a broad but shallow change, that changes a lot of files in fairly minor ways.
I want to be sure this didn't break any build, and especially any version of python from 3.9 onwards. |
# TODO(PROTON-2322) decide which of these two warnings we want disabled | ||
# _transport.py:908:21: W503 line break before binary operator | ||
# _transport.py:907:56: W504 line break after binary operator | ||
W504, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the w503, w504 should remain disabled, imo, otherwise it's not possible to have line breaks in expressions, which can be useful. But if it works out for you without this, then I guess it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rewrote the code to avoid using more than one line (and I think made it a little easier to understand)
And in general it's avoidable if you're checking before committing the change.
Tried a Python compatibility check in IntelliJ, and got
python 3.13 will be removing the https://docs.python.org/3.13/whatsnew/3.13.html#summary-release-highlights |
# ambiguous variable name 'l' | ||
E741, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't like this warning, would've kept it disabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that I wouldn't put it in a coding standard myself - but forbidding the letter 'l' to avoid confusion eith the number '1' has some sense IMO and it is in PEP8.
Again it's easy enough to avoid if you're checking before commit.
That's interesting - I'll look at that I wonder what we use it for. |
This is a broad but shallow change, that changes a lot of files in fairly minor ways.