Skip to content

Pre-commit config + fixes #21638

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

Closed
wants to merge 3 commits into from
Closed

Pre-commit config + fixes #21638

wants to merge 3 commits into from

Conversation

ianhi
Copy link
Contributor

@ianhi ianhi commented Nov 15, 2021

PR Summary

As discussed in #21583 (comment) this PR adds a pre-commit config and then:

  1. Fixed the docstring-first-check manually
  2. applies the autofixes from pre-commit

PR Checklist

Tests and Styling

  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

Update pre-commit hooks

rst format

Disable autofixes on PRs

Unless manually triggered. See https://pre-commit.ci/#configuration

add pre-commit to environment.yml

Slow down autoupdate schedule

alphabetize

Add sentence about where pre-commit config is

Add excludes to pre-commit config

Otherwise end of lines of test images will get fixed,
this also ignores all the prev_whats_new and prev_api_changes
as well as vendored components (agg, gitwash)

pre-comit config
@@ -60,6 +60,16 @@ true for ``*.py`` files. If you change the C-extension source (which might
also happen if you change branches) you will have to re-run
``python -m pip install -ve .``

Installing pre-commit hooks
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to talk about using them (ie. pre-commit run) instead of installing them? Personally I prefer not having pre-commit editing my files automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about adding a discussion of running them with out installing as hooks following this?

Something like:

If you do not want these git hooks you can skip the pre-commit install step. You can still run the checks with pre-commit run <list of files> or pre-commit run --all-files

Copy link
Member

Choose a reason for hiding this comment

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

👍 sounds good to me

Copy link
Contributor

Choose a reason for hiding this comment

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

I think pre-commit run ... will still update/modify the files if that is what the hook is doing though? It is just if you install the hooks automatically get run on every commit without a user running the command explicitly.

👍 to changing this section title to "Using pre-commit hooks" and expanding the text.

@tacaswell
Copy link
Member

Do we need to have this much white-space churn?

@ianhi
Copy link
Contributor Author

ianhi commented Nov 16, 2021

Do we need to have this much white-space churn?

As established when talking about black I'm a supporter of infinite amounts of churn... so that's a question I'm ill suited to answer. I think this rule is already enforced in py files by flake8 and this just extends it too all files. We could change that check to only run on py files.

@jklymak
Copy link
Member

jklymak commented Nov 16, 2021

When tracking what's going on in the codebase, it is nice if "git blame" points to substantive changes rather than whitespace "fixes"

@oscargus
Copy link
Member

When tracking what's going on in the codebase, it is nice if "git blame" points to substantive changes rather than whitespace "fixes"

But that is only going to happens once, right? Like when merging this.

(Or when people use editors that trim whitespaces.)

@jklymak jklymak marked this pull request as draft February 1, 2022 12:16
@jklymak
Copy link
Member

jklymak commented Feb 1, 2022

needs a rebase so I've put to draft - @ianhi, happy to add this to the weekly call if you want to see if this will go in or not. I think we talked about it previously and the general opinion was 👍, but not sure...

@QuLogic
Copy link
Member

QuLogic commented Jul 21, 2022

I'm pretty sure all of this has been applied in other PRs now.

@QuLogic QuLogic closed this Jul 21, 2022
@QuLogic QuLogic added status: duplicate and removed status: needs comment/discussion needs consensus on next step labels Jul 21, 2022
@ianhi ianhi deleted the pre-commit-all branch July 21, 2022 00:27
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.

7 participants