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

Dev: Add pre-commit.com framework hooks #7176

Merged
merged 8 commits into from Dec 10, 2020
Merged

Conversation

rambo
Copy link
Contributor

@rambo rambo commented Oct 23, 2020

For some basic good practises, see the framework for more info but TLDR is that it handles passing
only files that are in the commit to the hooks and it's very easy to manage hooks.

Fixed some executable flags while at it but all the whitespace normalizations and other changes that the hooks will do automatically will wait for whenever someone actually touches code so that we don't have a giant commit doing only those (as discussed in #7170 )

@rambo
Copy link
Contributor Author

rambo commented Oct 23, 2020

ping @Zen-CODE, here's the conservative version.

Copy link
Member

@matham matham left a comment

Choose a reason for hiding this comment

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

You may want to add to the docs , that --no-verify or -n can be used to temporarily disable the hook.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
- repo: https://github.com/Lucas-C/pre-commit-hooks
rev: v1.1.7
hooks:
- id: forbid-crlf
Copy link
Member

Choose a reason for hiding this comment

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

How does this work on Windows where git will auto convert crlf to lf before committing? Would it cause a error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None of my mates at work that are on windows have complained but I'll fire up a VM and doublecheck this personally. That'll probably happen tomorrow though (or maybe much later tonight is I can't sleep or something).

I'm pretty sure the lf<->crlf conversion that git does on windows is done in such way that the hook only sees the actual content going to repo (which would have been converted back to lf)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, this is getting way more complicated than just installing git and python for windows, what kind of env are the windows people actually running for development ?? If they're on WSL or cygwin they probably do not have the CRLF conversion even enabled if not how do they even enable the pre-commit hooks (ie where do they get make from, for example...)

Anyway gits own documentation says the conversion is done before committing and the hook sees the to-be-committed content so this shoud be transparent.

Copy link
Member

Choose a reason for hiding this comment

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

Personally I use msys and I set autocrlf=input.

Makefile Outdated Show resolved Hide resolved
Copy link
Member

@matham matham left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. It's a pretty innocuous change, but I'll wait until after the upcoming release/tag before merging on the off-chance it causes issues.

@matham matham merged commit df5b2c1 into kivy:master Dec 10, 2020
@matham matham changed the title Add pre-commit.com framework hooks Dev: Add pre-commit.com framework hooks Dec 10, 2020
@matham matham added the Component: tests/CI Tests, CI, GitHub settings label Dec 10, 2020
@matham matham added this to the 2.1.0 milestone Dec 10, 2020
hamlet4401 pushed a commit to tytgatlieven/kivy that referenced this pull request Jul 3, 2021
* Add pre-commit hooks for basic standards conformity checking

* These should not be marked as executable

* These lack shebangs so marking them executable did not do anyone any good

* add pre-commit to dev requirements

* use pre-commit framework for the hook

* Move the pre-commit config outside of root

As discusses in kivy#7176 (comment)

* Document that pre-commit is used and how to skip checks if needed

* Mark the old hook script as deprecated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: tests/CI Tests, CI, GitHub settings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants