-
Notifications
You must be signed in to change notification settings - Fork 64
Flake8 code style fixes #241
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
Conversation
0d9164c to
04d50ce
Compare
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.
Most of utility abstractions look good although it seems majority of if checks are now contradicting PEP8 style in the following two patterns:
if <var> is None:orif <var> is not Noneshould be preferred overif <var>:per PEP8 guidline - https://www.python.org/dev/peps/pep-0008/#programming-recommendationsif <var> == 0 or <some number>- not sure what's PEP8 stance on this type of comparisons, but numerical comparisons are better written this way to ensure better code readability and understanding instead ofif <var>:which seems like checking for None and may lead to confusion.
16f3c01 to
9b328f1
Compare
Most of utility abstractions look good although it seems majority of if checks are now contradicting PEP8 style in the following two patterns:
if <var> is None:orif <var> is not Noneshould be preferred overif <var>:per PEP8 guidline - https://www.python.org/dev/peps/pep-0008/#programming-recommendationsif <var> == 0 or <some number>- not sure what's PEP8 stance on this type of comparisons, but numerical comparisons are better written this way to ensure better code readability and understanding instead ofif <var>:which seems like checking for None and may lead to confusion.
You are right. Some variables are considered to be None by default, so it should be checked. Nice catch! I'll fix that.
In other cases, some of your checks, e.g., AF_PATH in library.py check if AF_PATH is None, but it should fail if AF_PATH == "". Simple conditions change to if AF_PATH can fix such problems when the default value or variable type is not known.
9b328f1 to
ad77595
Compare
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.
revert
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.
IMO, it's better now :)
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.
revert
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.
Still, I don't see a point to revert these changes. These variables are just temp variables and they are not changed later anyhow. But we drop extra 7 lines of code.
ad77595 to
6012303
Compare
6012303 to
28a0f61
Compare
c20136f to
911be16
Compare
911be16 to
b7cffa1
Compare
I'm going to postpone this PR till #244 is merged. I will add the arg --flake8 to the pytest for automatic code style checks here right after.
to_strmethod to library from utils to avoid import errors