Skip to content

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

@codecov
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
@Sauravroy34 Sauravroy34 deleted the pre_commit_hooks branch March 16, 2025 13:34
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