add pre-commit hooks for convenient formatting checks#4387
add pre-commit hooks for convenient formatting checks#4387pmeier merged 8 commits intopytorch:mainfrom
Conversation
|
There are some formatting failures, but I'll only push fixes if the PR is otherwise ready to go. |
There was a problem hiding this comment.
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
Co-authored-by: Nicolas Hug <[email protected]>
NicolasHug
left a comment
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks @pmeier , LGTM, I'm just wondering whether keeping the json hook was intended or not.
| pip install --user --progress-bar off flake8 typing | ||
| flake8 --config=setup.cfg . | ||
| pip install --user --progress-bar off pre-commit | ||
| pre-commit install-hooks |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 <[email protected]> Co-authored-by: Nicolas Hug <[email protected]>
4 participants
Addresses #4384 (comment). Right now we only use
flake8as format checker, but I've added a few more that I think are uncontroversial. Let me know if you think otherwise.cc @seemethere