Enable style checks #293
Enable style checks #293
Conversation
| @@ -30,7 +30,7 @@ deps = | |||
| pre-commit | |||
| commands = | |||
| pre-commit install-hooks | |||
| pre-commit run {posargs:--color always --all-files --show-diff-on-failure} | |||
| pre-commit run --color always --all-files --show-diff-on-failure {posargs} | |||
There was a problem hiding this comment.
Since the CI runs this with a trailing -- tox was skipping the important --all-files and instead trying to run on only non-committed files of which there were none.
There was a problem hiding this comment.
We could leave it there to always be green 😆
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #293 +/- ##
==========================================
+ Coverage 84.76% 85.19% +0.43%
==========================================
Files 44 46 +2
Lines 8542 8804 +262
==========================================
+ Hits 7241 7501 +260
- Misses 1301 1303 +2 ☔ View full report in Codecov by Sentry. |
braingram
force-pushed
the
enable_style_checks
branch
from
b9fb90f
to
5f94030
Compare
| @@ -12,8 +12,8 @@ repos: | |||
| - id: check-symlinks | |||
| - id: debug-statements | |||
| - id: detect-private-key | |||
| - id: end-of-file-fixer | |||
| - id: trailing-whitespace | |||
| # - id: end-of-file-fixer | |||
There was a problem hiding this comment.
Why are these removed? Don't we want these checks? And why comment them out, instead of deleting them?
There was a problem hiding this comment.
Doh, I had clicked auto-merge so this got merged as soon as you approved it. I can follow this up with a PR to remove them if that's what you'd like.
I commented them out because these are all failing and likely have been for some time. The PR that enabled them had a bug in the workflow where it ran the checks but never supplied any files to check (so it was checking nothing). See #293 (comment) for a comment on where this bug is fixed. Here's a PR that uncomments all the checks to show the extent of the failures:
#294
The reported diff for the checks is over 5000 lines long:
https://github.com/spacetelescope/stcal/actions/runs/11142692931/job/30966194873?pr=294
(and doesn't fix all the failures as many require manual updates).
I chose to not remove them because I assumed that the maintainers want these checks to run. If that's not the case I'd say we remove them entirely. If re-enabling the checks is preferred I was thinking it would be easiest to enable them perhaps one-by-one since each will require file changes.
There was a problem hiding this comment.
I'm not sure what's the best approach for this. It could be good to have them commented out, so we have explicit documentation for what is not used, but removing them to clean up the script could be good, too. I just wanted to make sure you wanted the comments left in the scripts.
| hooks: | ||
| - id: sp-repo-review | ||
| additional_dependencies: ["repo-review[cli]"] | ||
| # - repo: https://github.com/pre-commit/mirrors-prettier |
There was a problem hiding this comment.
Why is this commented out and not simply removed?
| args: ["--write-changes"] | ||
| additional_dependencies: | ||
| - tomli | ||
| # - repo: https://github.com/codespell-project/codespell |
There was a problem hiding this comment.
Why is this commented out instead of just removed?
| "W", "E", # pycodestyle (part of default flake8) | ||
| "I", # isort (import sorting) | ||
| select = [ | ||
| #"F", # Pyflakes (part of default flake8) |
There was a problem hiding this comment.
Why are these commented out instead of just removed?
Style checks (via pre-commit) have not been checking any files in this repo as part of the CI.
See this recent run on main:
https://github.com/spacetelescope/stcal/actions/runs/11017955113/job/30597119856#step:10:63
With all checks showing:
If I run them locally I see a large number of automatic changes:
36 files changed, 1091 insertions(+), 942 deletions(-)
and even after these see 79 ruff errors, many from codespell and 6 from reporeview.
In an effort to check something this PR:
check-stylejob to check filesWith this PR the checks run:
https://github.com/spacetelescope/stcal/actions/runs/11130795981/job/30930929199?pr=293#step:10:49
Tasks
docs/pageno-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)"git+https://github.com/<fork>/stcal@<branch>")jwstregression testromancalregression testnews fragment change types...
changes/<PR#>.apichange.rst: change to public APIchanges/<PR#>.bugfix.rst: fixes an issuechanges/<PR#>.general.rst: infrastructure or miscellaneous change