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

add pre-commit hooks for convenient formatting checks #4387

Merged
merged 8 commits into from
Sep 13, 2021

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Sep 10, 2021

Addresses #4384 (comment). Right now we only use flake8 as format checker, but I've added a few more that I think are uncontroversial. Let me know if you think otherwise.

cc @seemethere

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@pmeier
Copy link
Collaborator Author

pmeier commented Sep 10, 2021

There are some formatting failures, but I'll only push fixes if the PR is otherwise ready to go.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks @pmeier , it's a great idea to get the commit hooks rolling a bit before we include ufmt.

I made a few comments below, LMK what you think

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
pmeier and others added 2 commits September 10, 2021 12:44
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
@pmeier pmeier requested a review from NicolasHug September 10, 2021 10:55
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks @pmeier , LGTM, I'm just wondering whether keeping the json hook was intended or not.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@pmeier pmeier requested a review from NicolasHug September 13, 2021 08:55
pip install --user --progress-bar off flake8 typing
flake8 --config=setup.cfg .
pip install --user --progress-bar off pre-commit
pre-commit install-hooks
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can delete this line?
I think run --all-files should work even without having the hooks? At least that's what our instructions in the README suggest.

CI failures are unrelated BTW, it's good to merge on my side. I'll let you do it in case there's any leftovers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like to keep the actual check separate from the installation. This way, the last step in the workflow only contains the information that a contributor needs in case the workflow fails. Granted, the installation of the hook environments does not create a lot of noise, but still.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pmeier pmeier merged commit bc3f8f6 into pytorch:main Sep 13, 2021
@pmeier pmeier deleted the pre-commit-hooks branch September 13, 2021 10:00
facebook-github-bot pushed a commit that referenced this pull request Sep 30, 2021
Summary:
* add pre-commit hooks

* ignore yamls in packaging/*

* add pre-commit to contributing guide lines

* Update CONTRIBUTING.md

* remove some hooks

* fix docstrings

* fix end of files

Reviewed By: datumbox

Differential Revision: D31268049

fbshipit-source-id: 6483fb766d86965b0a797b06f983c1f36e8bc1aa

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
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.

4 participants