Skip to content
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

enforce Python style #1289

Open
KrisThielemans opened this issue Nov 17, 2023 · 3 comments
Open

enforce Python style #1289

KrisThielemans opened this issue Nov 17, 2023 · 3 comments

Comments

@KrisThielemans
Copy link
Collaborator

#98 mostly talks about C++, but we need this for Python as well, which clang-format doesn't do.

Current status for STIR: #724 and #970 installed all/most infrastructure for clang-format, including pre-commit config. We still haven't run this, pending imminent merge of the TOF PR. Current doc on the process is at https://github.com/UCL/STIR/blob/master/documentation/devel/README.md

Some info from @casperdcl

black: Python & extremely opinionated by design (can only config line length) so I don't tend to use it at all
yapf: Python & configurable
isort: Python import sorting (compatible with above tools)
Also worth mentioning static tools like flake8.
I tend to put config in .pre-commit-config.yaml and pyproject.toml.
I'd also recommend using https://pre-commit.ci which automatically does CI & opens correctional PRs (without needing any additional config).

Example file https://github.com/TomographicImaging/eqt/blob/main/.pre-commit-config.yaml.

Note that we run Codacy which runs Bandit, Prospector, Pylint, but this leaves manual intervention by the user, so it's better to do this via pre-commit of course.

Suggested process:

  1. PR adding hooks/config/doc, include link to https://github.com/google/yapf/blob/main/EDITOR%20SUPPORT.md in our EDITOR_SUPPORT.md (and others?)
  2. merge TOF PR to master
  3. PR run pre-commit and git commit --author="pre-commit-format <[email protected]>"
  4. use https://pre-commit.ci
  5. tell everone this will now be enforced, but recommend to set editors accordingly. Ideally we also clean-up/confirm the process by @ashgillman in configuration for white-space #724 (comment) and its follow-up
@casperdcl
Copy link
Collaborator

casperdcl commented Nov 17, 2023

@KrisThielemans
Copy link
Collaborator Author

great. I guess after enabling pre-commit.ci, we create a PR only updates the doc, or maybe it'll run first on master. that'll be obvious.

@robbietuk
Copy link
Collaborator

I frequently use Black and MyPy in .pre-commit-config.yaml

black: Python & extremely opinionated by design (can only config line length) so I don't tend to use it at all

I don't see black being opinionated and strict as a bad thing, it enforces uniformity as default.

It is also worth mentioning MyPy for static typing. Though, I am not sure it would be useful for STIR as most of the python usages are small script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants