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

Proposal to use pre-commit for continuous integration #1240

Merged
merged 2 commits into from Nov 13, 2023
Merged

Conversation

dachengx
Copy link
Collaborator

@dachengx dachengx commented Aug 24, 2023

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:

  • Update the docstring(s)
  • Update the documentation
  • Tests to check the (new) code is working as desired.
  • Does it solve one of the open issues on github?

Inspired by and closes #1192

@coveralls
Copy link

coveralls commented Aug 24, 2023

Coverage Status

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

@dachengx
Copy link
Collaborator Author

dachengx commented Sep 29, 2023

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

@dachengx dachengx mentioned this pull request Oct 4, 2023
Copy link
Collaborator

@WenzDaniel WenzDaniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dachengx
Copy link
Collaborator Author

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
Copy link
Collaborator Author

The test is passed. I will merge the PR.

@dachengx dachengx merged commit 4dcae6f into master Nov 13, 2023
9 checks passed
@dachengx dachengx deleted the use_pre-commit branch November 13, 2023 17:03
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

Successfully merging this pull request may close these issues.

Use https://pre-commit.com/
3 participants