StingraySoftware / stingray

Anything can happen in the next half hour (including spectral timing made easy)!
https://stingray.science/stingray
MIT License
172 stars 141 forks source link

pre_commit hooks #847

Closed Sauravroy34 closed 3 days ago

Sauravroy34 commented 2 weeks ago

Added some pre-commit hooks

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

matteobachetti commented 1 week ago

@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 commented 1 week ago

@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[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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 ```diff @@ 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](https://app.codecov.io/gh/StingraySoftware/stingray/pull/847/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StingraySoftware) | Coverage Δ | | |---|---|---| | [](https://app.codecov.io/gh/StingraySoftware/stingray/pull/847/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StingraySoftware) | `78.85% <ø> (-17.54%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StingraySoftware#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Sauravroy34 commented 1 week ago

@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 commented 6 days ago

@matteobachetti i have added some docs for pre-commit