-
Notifications
You must be signed in to change notification settings - Fork 226
Generalize Analysis Scripts: Number of Digits (plt) #2683
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
7a00d75 to
f1c808a
Compare
Thanks Phil! :) Co-authored-by: Phil Miller <[email protected]>
| import sys | ||
| import yt | ||
| import numpy as np | ||
| import os |
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.
To be PEP8 compliant, the import os should be before the imports of numpy and yt.
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.
Interesting PEP8 style choice, because contrary to C/C++ includes, this has no real influence ^^
(besides global behavior, like where mpi4py is placed).
https://www.python.org/dev/peps/pep-0008/#imports
I think we need a bot for this, maybe we can add this to .pre-commit-config.yaml.
I would not change it in the current PR, since already existing sys imports are mostly off. Let's do style fixes to existing code in separate style-only/cleanup PRs?
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.
Clarified on Slack: separate, we are looking for tools that can do it :)
Maybe autopep8 or flake8.
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 tried autopep8 --select E401,E402 --in-place --aggressive --aggressive <filename> and all it does is sort alphabetically.
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.
The tool isort does the trick: #2686
https://github.com/PyCQA/isort
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.
Looks good to me, thanks for splitting #2385 in smaller tasks.
Preparation for #2385: Generalize a list of test scripts so that we have an easier time finishing that long-standing PR with less conflicts to track.
Also avoids that we add more scripts with the old/hard-coded patterns.