-
Notifications
You must be signed in to change notification settings - Fork 510
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
coding style for Python #2002
Comments
Does strict adherence to PEP really impact readability that much? In my opinion, the names of variables, code comments, and code architecture influence code readability maybe an order of magnitude more. With C++ it's a bit difference since you have complete freedom to do wacko stuff with whitespace. On the other hand, there are some basic aesthetic and practical consistency aspects such as using spaces rather than tabs, with four spaces per "tab", not putting spaces between the equal sign and kwargs, and not grossly exceeding 80 characters per line. These are things that stick out like a sore thumb if not adhered to in a PR. Other aspects of PEP such as the rules around how to indent multi-line arguments passed into a function are completely unnecessary and impact readability in no way IMO. Just my $0.02! |
Yep, I agree that the code is nice and readable at the moment and that automating PEP on the code might not improve the readability significantly. Black would do those basic aesthetic aspects that you mention such has conversion of tabs to spaces, remove spaces between equals signs and kwargs and can wrap lines above a specified length. For me the important part is that all this would be done automatically without the reviewer having to waste time doing these corrections. |
Agree with @shimwell here, black is the standard python style and drastically simplifies decisions about formatting. |
We implemented something similar on PyNe a while back which might be relevant because in that case we formatted a large amount of code. First we made and merged a single PR that formatted the entire code base The we made a PR that helped keep the code formatted consistently for future PRs One other change we could do this file by file and then protect each file that has been formatted with the CI tests instead of all at once. The first PyNe PR was 22k of line changes and this was perhaps a bit big. I can make this PR just to show more clearly |
I have made an example PR with a python-format-checking CI that checks a file is well formatted. The CI originally fails, then once the file is formatted with black then the CI passes. this allows us to bring files under the formatting rule one file at a time but perhaps is this overly cautious what do people think |
digging up some old thread. I would recommend |
Just wanted to make note that the popular ruff package now supports formatting. |
Just to mention I've been making use of darker to format just the parts of the file I've been changing prior to submitting PRs. It runs black on just the part of the code changed, partial formatting |
@paulromano is suggesting a Rust utility? So I have played around with this a little bit on my own branch. Here's my notes:
My two cents:
|
Could we have a coding style for Python using in OpenMC such as Black?
We have a clang coding style for the C++ part of the code and a .clang_format as described in the docs here and here so this would be a similar situation for the Python side.
Currently a
flake8
on the develop branch scan picks up 2635 pep8 typosThis could be brought down a bit if we run a code formatter on the code like this
I notice sometimes we use " " and other times we use ' ' quotes so we could use the
--skip-string-normalization
to avoid changing the string quotes if preferred?Then we could also add a github action to automatically keep the code formatted nicely in the future. This one will format the code and commit back to the feature branch and skip the CI (to avoid running it twice)
And finally we could add a pre-commit file name
.pre-commit-config.yaml
to allow developers to automatically format prior to committingThere is at least one part of the code that I would refactor as a comment before applying these sweeping changes as in the assembly example we use white space in the code to indicate location of fuel pins. I would make a comment with the pin layout and then let black format the code.
I would be happy to put in PR(s) for these changes or part of these changes if they are welcome?
The text was updated successfully, but these errors were encountered: