XENONnT / straxen

Streaming analysis for XENON
BSD 3-Clause "New" or "Revised" License
20 stars 32 forks source link

Proposal to use pre-commit for continuous integration #1240

Closed dachengx closed 10 months ago

dachengx commented 1 year ago

What does the code in this PR do / what does it improve?

Stop using the wemake-python-styleguide which gives too many errors. And switch to pre-commit's continuous integration.

The advantage of pre-commit is that:

  1. It runs automatically when you git commit.
  2. You will not upload codes that are not aligned with some styles.

The codes are changed only in code style, the requirement of hooks.

Note: black will force you to use double quotes, it actually auto-format for you, so do not worry.

You will see no reviewdog complaining, and pre-commit will do almost all formatting for you, and it is very fast.

Can you briefly describe how it works?

To setup pre-commit:

cd straxen
pip install pre-commit
pre-commit install

Besides basic whitespace and large files check,

  1. black will auto-format your code, mainly about indent and line-length, and make auto-commit if necessary.
  2. docformatter will auto-format your docstring, following google convention, and make auto-commit if necessary.
  3. flake8 will check the code-style, but in a more lite way than wemake-python-styleguide.

So there will be two pre-commit checks, one check of changed files when you git commit, and one check of all files by pre-commit CI.

Can you give a minimal working example (or illustrate with a figure)?

When you git commit, it will automatically pre-commit run for the changed files, so actually you would not always run it manually.

If you want to check all the files, run pre-commit run --all-files. If you want to check the difference between the current branch and main, run pre-commit run --from-ref main --to-ref .. If you want to remove the color from the output, add --color never.

black and docformatter will change your code, you still need to make a commit by yourself on the changed part.

For more information, please go to https://pre-commit.com/ and https://pre-commit.ci/.

Please include the following if applicable:

Inspired by and closes https://github.com/XENONnT/straxen/issues/1192

coveralls commented 1 year ago

Coverage Status

coverage: 92.785% (+0.2%) from 92.611% when pulling af309b51241306cc6f94124c25c6bd3a0b8e25cc on use_pre-commit into 8da67571607f12b54f8954c5829429562090ab9f on master.

dachengx commented 11 months ago

Most importantly, no lineage is changed. I will then test detailedly about the change of each field.

dachengx commented 10 months ago

For me it looks fine, thanks @dachengx. Do you know why some of the test are currently failing is it because of the LNGS network thing? If yes lets maybe wait and rerun the tests when it is back up, just to be sure.

Yes, it is related to https://xenonnt.slack.com/archives/C016DM0JPK9/p1699638121051979, a network issue. I will rerun the test after the issue is gone.

dachengx commented 10 months ago

The test is passed. I will merge the PR.