UCL / STIR

Software for Tomographic Image Reconstruction
http://stir.sourceforge.net/
Other
114 stars 95 forks source link

enforce Python style #1289

Open KrisThielemans opened 11 months ago

KrisThielemans commented 11 months ago

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 <noreply@github.com>"
  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 https://github.com/UCL/STIR/pull/724#issuecomment-848435651 and its follow-up
casperdcl commented 11 months ago
KrisThielemans commented 11 months ago

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 commented 8 months ago

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.