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.

Sign up for GitHub

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

Jump to bottom

Add precommit #76

Merged
merged 4 commits into from
Dec 19, 2021
Merged

Add precommit #76

merged 4 commits into from
Dec 19, 2021

Conversation

SauravMaheshkar
Copy link
Contributor

Fixes #75.

Based on the feedback provided by @streeve I've added black among some other miscellaneous checks to the .pre-commit-config.yaml. I've also added pre-commit to the requirements-dev.txt file in order to aid new contributors.

I have not added pytest as of right now based on the feedback provided but more than happy to take that up again.

I'd also recommend adding instructions on how to install pre-commit hooks to the CONTRIBUTING.md file.

@streeve streeve self-requested a review December 17, 2021 21:32
streeve
streeve approved these changes Dec 17, 2021
View reviewed changes
Copy link
Collaborator

@streeve streeve left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for contributing. If there is a way to add pytest, but turn it off by default, I think it would also be very useful. But that does not necessarily need to be done in the same PR

.pre-commit-config.yaml
- id: end-of-file-fixer

- repo: https://github.com/psf/black
rev: 21.5b1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I changed this just to match the current version enforced in the CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's the option of turning off git hooks with git commit --no-verify -m '<insert-commit-message>'. But that's a hack IMO. I didn't pytest because I noticed that the repository already has CI that runs on every PR which also runs pytest.

@streeve streeve requested review from allaffa and pzhanggit December 17, 2021 21:40
@SauravMaheshkar
Copy link
Contributor Author

PS: Apparently there are some files which are corrected by black. Maybe I can do a follow up PR ? This PR would just contain the changes after running black on all files.

@allaffa allaffa added the enhancement New feature or request label Dec 19, 2021
@allaffa allaffa merged commit 6c04292 into ORNL:main Dec 19, 2021
@SauravMaheshkar SauravMaheshkar deleted the add-precommit branch December 19, 2021 08:06
RylieWeaver pushed a commit to RylieWeaver/HydraGNN that referenced this pull request Oct 17, 2024
@SauravMaheshkar @streeve
Add precommit (ORNL#76)
08df317 
* Add pre-commit

* Add pre-commit to dev requirements

* Freeze black required version for now

* Add instructions for using pre-commit

Co-authored-by: Sam Reeve <6740307+streeve@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@streeve streeve streeve approved these changes

@allaffa allaffa allaffa approved these changes

@pzhanggit pzhanggit Awaiting requested review from pzhanggit

Assignees

@SauravMaheshkar SauravMaheshkar

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add pre-commit to make contributing easy.
3 participants
@SauravMaheshkar @allaffa @streeve