Skip to content

Upstream pre commit config #15

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

Merged
merged 3 commits into from
Apr 30, 2021
Merged

Upstream pre commit config #15

merged 3 commits into from
Apr 30, 2021

Conversation

josephtate
Copy link
Contributor

This PR sets up pre-commit on the repo itself. It enables a few simple tests, and then turns on flake8 on commit.

Docs are also updated to simplify the installation, and to instruct developers on how to set up pre-commit themselves.

@milind-shakya-sp
Copy link
Collaborator

Thanks! Will take a look soon.

README.rst Outdated
@@ -62,8 +62,7 @@ You need to have precommit setup to use this hook.
::

pip install pre-commit
pre-commit install
pre-commit install --hook-type commit-msg
pre-commit install --install-hooks
Copy link
Owner

Choose a reason for hiding this comment

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

This won't work. Need to explicitly run pre-commit install --hook-type commit-msg otherwise giticket wont work. needs to be reverted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like you're right. I expected that --install-hooks was minimally sufficient, but it doesn't actually install any hooks, just sets up the envs for the existing hooks.

CONTRIBUTING.rst Outdated
4. Set up pre-commit::

$ pip install pre-commit
$ pre-commit install --install-hooks
Copy link
Owner

Choose a reason for hiding this comment

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

Would add pre-commit install --hook-type commit-msg step here as well

Copy link
Owner

@milin milin left a comment

Choose a reason for hiding this comment

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

Need to mention running pre-commit install --hook-type commit-msg for new users.

@josephtate
Copy link
Contributor Author

Need to mention running pre-commit install --hook-type commit-msg for new users.

I've restored the original instructions, and filed pre-commit/pre-commit#1895 to have --install-hooks do the expected thing.

Copy link
Collaborator

@milind-shakya-sp milind-shakya-sp left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Looks great

@milin milin merged commit bd0d7c6 into milin:master Apr 30, 2021
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.

3 participants