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

[INFRA] add (optional) pre-commit config file #929

Merged
merged 3 commits into from
Nov 19, 2021

Conversation

sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented Nov 15, 2021

closes #928 the proposal by @Remi-Gau (with which I agree)

people can optionally use this by doing:

  1. pip install pre-commit and then
  2. pre-commit install (from the root of the repo)

We could potentially even run the hooks in the CI and raise an error if any fail 🤔

Like here: https://github.com/executablebooks/sphinx-copybutton/blob/6ee971ab8f9c3741d380240c180b1ff47f1d2ddc/.github/workflows/integration.yml#L22-L24

Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

Are the file changed a result of running pre-commit run -a?

Maybe we should mention the use of pre-commit in the CONTRIBUTING.md?

@sappelhoff
Copy link
Member Author

Are the file changed a result of running pre-commit run -a?

yes, but if we do it in CI, we don't commit those changes --> we would just see if there are any, and then raise if True.

The argument against adding pre-commit to the CI is that it might confuse contributors and take away a bit of the "optionality" of pre-commit as we want to introduce it right now

Maybe we should mention the use of pre-commit in the CONTRIBUTING.md?

will do

@Remi-Gau
Copy link
Collaborator

The argument against adding pre-commit to the CI is that it might confuse contributors and take away a bit of the "optionality" of pre-commit as we want to introduce it right now

yeah for the moment I would hold back from using the pre-commit CI: it is great and uber fast but it can really confuse people if they don't have pre-commit installed locally.

Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

This looks good to me. I can't wait to have this merged in.

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Looks reasonable

requirements.txt Outdated Show resolved Hide resolved
@sappelhoff sappelhoff merged commit d5b64ba into bids-standard:master Nov 19, 2021
@sappelhoff sappelhoff deleted the add/precommit branch November 19, 2021 10:51
@effigies effigies added the exclude-from-changelog This item will not feature in the automatically generated changelog label Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude-from-changelog This item will not feature in the automatically generated changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use pre-commit to lint files upon commit
3 participants