-
Notifications
You must be signed in to change notification settings - Fork 144
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 hooks #847
pre_commit hooks #847
Conversation
@Sauravroy34 thanks. I'm a little hesitant to use extensive pre-commit hooks, in particular when they mess a lot with the code. |
Sure there might be a problem with using black since there might be a chance that it tweaks some essential logic. and end of files and trailing whitespace are safer |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #847 +/- ##
===========================================
- Coverage 96.39% 78.85% -17.54%
===========================================
Files 48 48
Lines 9292 9290 -2
===========================================
- Hits 8957 7326 -1631
- Misses 335 1964 +1629
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thanks @Sauravroy34 !
What should the user/developer do to use these hooks? If any action from the user is needed (e.g. the installation of some tool), then the developer documentation should be updated accordingly. In any case, the use of pre-commit hooks should be documented if we want to avoid giving the users some inconsistent behavior (e.g. files modified by the hook while they are editing them).
@matteobachetti I see the concern over here maybe to ensure that pre-commit is run every time a pr pops up (like in astropy and sunpy). So we can have it added in GitHub workflow (pre-commit.ci https://pre-commit.ci/ ) and we can have auto fix as false so it does not fixes automatically |
@matteobachetti i have added some docs for pre-commit |
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.
Ok, let's see how it goes. Thanks!
One small additional request: this certainly deserves a changelog entry!
Please add a file to docs/changes named 847.trivial.rst
with a short changelog entry explaining the addition of pre-commit hooks, following the towncrier instructions .
21997f5
Added some pre-commit hooks
fixes #815 so far added for black,whitespace and flake8