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.

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

Merged
merged 11 commits into from
Sep 30, 2024
Merged

Conversation

Sauravroy34
Copy link
Contributor

Added some pre-commit hooks

fixes #815 so far added for black,whitespace and flake8

@matteobachetti
Copy link
Member

@Sauravroy34 thanks.

I'm a little hesitant to use extensive pre-commit hooks, in particular when they mess a lot with the code.
Frankly, of all these I would only keep the trailing whitespace one, which is safe in most situations I can think of.
The black one might make sense because we have the CI test, but I don't want to force the use in all cases, there might be situations where we can relax the requirement, while the pre-commit hook would force black on the code in all cases.
I would certainly not use the flake8 one, in particular because it might conflict with black.

@Sauravroy34
Copy link
Contributor Author

@Sauravroy34 thanks.

I'm a little hesitant to use extensive pre-commit hooks, in particular when they mess a lot with the code. Frankly, of all these I would only keep the trailing whitespace one, which is safe in most situations I can think of. The black one might make sense because we have the CI test, but I don't want to force the use in all cases, there might be situations where we can relax the requirement, while the pre-commit hook would force black on the code in all cases. I would certainly not use the flake8 one, in particular because it might conflict with black.

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

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.85%. Comparing base (c3e6dd3) to head (a0dc096).

❗ There is a different number of reports uploaded between BASE (c3e6dd3) and HEAD (a0dc096). Click for more details.

HEAD has 15 uploads less than BASE
Flag BASE (c3e6dd3) HEAD (a0dc096)
16 1
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     
Flag Coverage Δ
78.85% <ø> (-17.54%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@matteobachetti matteobachetti left a 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).

@Sauravroy34
Copy link
Contributor Author

Sauravroy34 commented Sep 25, 2024

@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

@Sauravroy34
Copy link
Contributor Author

@matteobachetti i have added some docs for pre-commit

Copy link
Member

@matteobachetti matteobachetti left a 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 .

@matteobachetti matteobachetti added this pull request to the merge queue Sep 30, 2024
Merged via the queue into StingraySoftware:main with commit 21997f5 Sep 30, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement pre-commit Hooks
2 participants