-
Notifications
You must be signed in to change notification settings - Fork 7.2k
More instructions in contributing guide regarding formatting #4536
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general LGTM. I've added a few inline comments.
Feel free to read the [pre-commit docs](https://pre-commit.com/#usage) to learn | ||
more and improve your workflow. You'll see for example that `pre-commit run | ||
--all-files` will run both `ufmt` and `flake8` without the need for you to | ||
commit anything, and that the `--no-verify` flag can be added to `git commit` to | ||
temporarily deactivate the hooks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that someone who wants to know more about the tools they are using to do that without extra instructions. Given that we only describe how to format / check the code here, I think this can be safely removed. No strong opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this part, I mostly wanted to mention the pre-commit run --all-files
command and the --no-verify
flag, which I think are important to know for regular development.
Then it becomes a matter of linking to the docs or not, I feel like there's no harm in encouraging contributors to check them?
Thanks @pmeier for the review. Is it good to go on your side? @fmassa @datumbox @prabhat00155 do these instructions make sense? Is there anything you think is missing or unclear? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks @NicolasHug!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, let's merge!
…#4536) Summary: * Enhanced contributing guide * Some more * Address comments Reviewed By: NicolasHug Differential Revision: D31505563 fbshipit-source-id: f791d8115c1d88a7b4e3bb4736f4eac8faf44f0b
…#4536) * Enhanced contributing guide * Some more * Address comments
…#4536) * Enhanced contributing guide * Some more * Address comments
LMK what you think @pmeier