-
Notifications
You must be signed in to change notification settings - Fork 2
pre-commit #173
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
pre-commit #173
Conversation
zachcran
left a comment
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 have a few minor requested changes and comments. Overall, looks good!
|
@jwaldrop107 How would you like me to provide feedback on the |
Co-authored-by: Zachery Crandall <[email protected]>
|
@zachcran We can discuss it here, or you can start an issue in the |
|
I'll open an issue for discussion there since most/all of it probably won't affect this PR's contents. |
|
@jwaldrop107 I opened the following issue for |
zachcran
left a comment
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 left some suggestions, but nothing necessary to block merging, so I am approving and leaving it up to you if you implement the suggestions or not.
|
🚀 [bumpr] Bumped! |
Is this pull request associated with an issue(s)?
No.
Description
This PR adds the instructions for configuring
pre-commithooks using the configuration file found NWChemEx/pre-commit-config (and why the config lives in that repo). Most of the (rather large amount of) changes in this PR are a result of applying license headers and trimming whitespace courtesy of thepre-commithooks. The main point of review is the new additions to the developer documentation found indocs/source/conventions/pre_commit.rst. Relatedly, the reviewers should take a look at the .pre-commit-config.yaml in NWChemEx/pre-commit-config.