STIR icon indicating copy to clipboard operation
STIR copied to clipboard

enforce Python style

Open KrisThielemans opened this issue 1 year ago • 3 comments

#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 https://github.com/UCL/STIR/pull/724#issuecomment-848435651 and its follow-up

KrisThielemans avatar Nov 17 '23 13:11 KrisThielemans

  • example links are already in "I tend to put config in .pre-commit-config.yaml and pyproject.toml" which has both C++ & Python (unlike eqt)
  • step (3) "run pre-commit && git commit" isn't required as (4) https://pre-commit.ci will open a PR authored by the bot anyway

casperdcl avatar Nov 17 '23 15:11 casperdcl

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.

KrisThielemans avatar Nov 17 '23 15:11 KrisThielemans

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.

robbietuk avatar Feb 15 '24 16:02 robbietuk