Skip to content

Use pre-commit package to do codestyle CI checks#9890

Closed
Cadair wants to merge 2 commits into
astropy:masterfrom
Cadair:pre-commit-codestyle
Closed

Use pre-commit package to do codestyle CI checks#9890
Cadair wants to merge 2 commits into
astropy:masterfrom
Cadair:pre-commit-codestyle

Conversation

@Cadair

@Cadair Cadair commented Jan 24, 2020

Copy link
Copy Markdown
Member

The objective of this PR is to suggest a solution to the problem discussed at the coordination meeting of how can we make it easy for people to ensure that their code is conformant to the CIs enforcement of the style before they push.

This does two things:

  1. Specify the code style checks using the pre-commit package.
  2. Run them in the codestyle tox env.

Using the pre-commit hooks locally as a git pre-commit is entirely optional, but doing so means you will always pass that CI build. If you don't want the git commit but want to check your code if conformant, you can run pre-commit run --all-files.

Also, I enabled a bunch of extra style checks, like trailing whitespace and blank lines at end of files etc. Fixing these is the large part of this diff.

@astrofrog

Copy link
Copy Markdown
Member

Just to make sure I understand how this works, how does git know about this pre-commit hook?

Comment thread tox.ini
pre-commit
commands =
pre-commit install-hooks
pre-commit run --all-files

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

couldn't this be the other way around, for pre-commit to run the tox command?

E.g. looking into the tox config is more intuitive thing to do to look for the exact error codes than yet at another place.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree, is there any way to switch this around?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That certainly doesn't work out of the box. Also pre-commit comes with a load of other checks plugins other than flake8, so it's a simple way to configure multiple checks.

@Cadair

Cadair commented Jan 25, 2020

Copy link
Copy Markdown
Member Author

Just to make sure I understand how this works, how does git know about this pre-commit hook?

If you want to teach git about it you can run

pre-commit install

you have to do this for each checkout you want to run the pre-commit for (git limitation).

@Cadair Cadair force-pushed the pre-commit-codestyle branch from e2554f8 to 68d71cc Compare January 29, 2020 20:14
@bsipocz bsipocz added this to the v4.0.1 milestone Jan 29, 2020
@bsipocz bsipocz modified the milestones: v4.0.1, v4.1 Jan 29, 2020
@mhvk mhvk removed this from the v4.1 milestone May 3, 2020
@mhvk

mhvk commented May 3, 2020

Copy link
Copy Markdown
Contributor

Not release-critical, so removing milestone

@astropy-bot

astropy-bot Bot commented Jul 3, 2020

Copy link
Copy Markdown

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

@Cadair Cadair closed this Jul 3, 2020
@mhvk mhvk mentioned this pull request Feb 27, 2021
24 tasks
@nstarman

Copy link
Copy Markdown
Member

@Cadair How can I help get this restarted / in ?

@saimn saimn mentioned this pull request Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants