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.

Sign up for GitHub

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

Enable style checks #293

Merged
merged 3 commits into from
Oct 2, 2024
+38 −52
Merged

Enable style checks #293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
635f91b
fix style check
braingram Oct 1, 2024
6faa3c5
skip failing ruff/pre-commit checks
braingram Oct 1, 2024
5f94030
skip more failing pre-commit checks
braingram Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 15 additions & 21 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these removed? Don't we want these checks? And why comment them out, instead of deleting them?

Copy link
Collaborator Author

@braingram braingram Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# - id: trailing-whitespace

- repo: https://github.com/pre-commit/pygrep-hooks
rev: v1.10.0
Expand All @@ -22,26 +22,26 @@ repos:
- id: rst-inline-touching-normal
- id: text-unicode-replacement-char

- repo: https://github.com/codespell-project/codespell
rev: v2.2.5
hooks:
- id: codespell
args: ["--write-changes"]
additional_dependencies:
- tomli
# - repo: https://github.com/codespell-project/codespell
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented out instead of just removed?

# rev: v2.2.5
# hooks:
# - id: codespell
# args: ["--write-changes"]
# additional_dependencies:
# - tomli

- repo: https://github.com/astral-sh/ruff-pre-commit
rev: "v0.1.5"
hooks:
- id: ruff
args: ["--fix", "--show-fixes"]
- id: ruff-format
# - id: ruff-format

- repo: https://github.com/MarcoGorelli/cython-lint
rev: v0.15.0
hooks:
- id: cython-lint
- id: double-quote-cython-strings
# - id: double-quote-cython-strings

- repo: https://github.com/pycqa/isort
rev: 5.12.0
Expand All @@ -57,13 +57,7 @@ repos:
additional_dependencies:
- black==22.12.0

- repo: https://github.com/pre-commit/mirrors-prettier
rev: "v3.0.1"
hooks:
- id: prettier

- repo: https://github.com/scientific-python/cookie
rev: 2023.10.27
hooks:
- id: sp-repo-review
additional_dependencies: ["repo-review[cli]"]
# - repo: https://github.com/pre-commit/mirrors-prettier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented out and not simply removed?

# rev: "v3.0.1"
# hooks:
# - id: prettier
52 changes: 22 additions & 30 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,50 +120,50 @@ src = [
]

[tool.ruff.lint]
extend-select = [
"F", # Pyflakes (part of default flake8)
"W", "E", # pycodestyle (part of default flake8)
"I", # isort (import sorting)
select = [
#"F", # Pyflakes (part of default flake8)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these commented out instead of just removed?

#"W", "E", # pycodestyle (part of default flake8)
#"I", # isort (import sorting)
# "N", # pep8-naming
"D", # pydocstyle (docstring style guide)
"UP", # pyupgrade (upgrade code to modern python)
#"D", # pydocstyle (docstring style guide)
#"UP", # pyupgrade (upgrade code to modern python)
"YTT", # flake8-2020 (system version info)
"ANN", # flake8-annotations (best practices for type annotations)
"S", # flake8-bandit (security checks)
#"S", # flake8-bandit (security checks)
"BLE", # flake8-blind-except (prevent blind except statements)
"B", # flake8-bugbear (prevent common gotcha bugs)
#"B", # flake8-bugbear (prevent common gotcha bugs)
"A", # flake8-builtins (prevent shadowing of builtins)
"C4", # flake8-comprehensions (best practices for comprehensions)
"T10", # flake8-debugger (prevent debugger statements in code)
"EM", # flake8-errormessages (best practices for error messages)
#"EM", # flake8-errormessages (best practices for error messages)
"FA", # flake8-future-annotations (correct usage future annotations)
"ISC", # flake8-implicit-str-concat (prevent implicit string concat)
"ICN", # flake8-import-conventions (enforce import conventions)
"G", # flake8-logging-format (best practices for logging)
#"G", # flake8-logging-format (best practices for logging)
"INP", # flake8-no-pep420 (prevent use of PEP420, i.e. implicit name spaces)
"PIE", # flake8-pie (misc suggested improvement linting)
#"PIE", # flake8-pie (misc suggested improvement linting)
# "T20", # flake8-print (prevent print statements in code)
"PT", # flake8-pytest-style (best practices for pytest)
"Q", # flake8-quotes (best practices for quotes)
#"PT", # flake8-pytest-style (best practices for pytest)
#"Q", # flake8-quotes (best practices for quotes)
"RSE", # flake8-raise (best practices for raising exceptions)
"RET", # flake8-return (best practices for return statements)
"SLF", # flake8-self (prevent private member access)
#"RET", # flake8-return (best practices for return statements)
#"SLF", # flake8-self (prevent private member access)
"SLOT", # flake8-slots (require __slots__ for immutable classes)
"SIM", # flake8-simplify (suggest simplifications to code where possible)
#"SIM", # flake8-simplify (suggest simplifications to code where possible)
"TID", # flake8-tidy-imports (prevent banned api and best import practices)
"TCH", # flake8-type-checking (move type checking imports into type checking blocks)
"INT", # flake8-gettext (when to use printf style strings)
# "ARG", # flake8-unused-arguments (prevent unused arguments)
"PTH", # flake8-use-pathlib (prefer pathlib over os.path)
#"PTH", # flake8-use-pathlib (prefer pathlib over os.path)
# "ERA", # eradicate (remove commented out code)
"PGH", # pygrep (simple grep checks)
"PL", # pylint (general linting, flake8 alternative)
"TRY", # tryceratops (linting for try/except blocks)
#"PL", # pylint (general linting, flake8 alternative)
#"TRY", # tryceratops (linting for try/except blocks)
"FLY", # flynt (f-string conversion where possible)
"NPY", # NumPy-specific checks (recommendations from NumPy)
"PERF", # Perflint (performance linting)
#"NPY", # NumPy-specific checks (recommendations from NumPy)
#"PERF", # Perflint (performance linting)
"LOG",
"RUF", # ruff specific checks
#"RUF", # ruff specific checks
]
ignore = [
"ISC001", # interferes with formatter
Expand Down Expand Up @@ -236,14 +236,6 @@ line_length = 110
[tool.codespell]
skip = "*.pdf,*.fits,*.asdf,.tox,build,./tags,.git,docs/_build"

[tool.repo-review]
ignore = [
"GH200", # Use dependabot
"PC140", # add MyPy to pre-commit
"PC901", # custom pre-comit.ci message
"MY100", # Use MyPy
]

[tool.cibuildwheel.macos]
archs = [
"x86_64",
Expand Down
2 changes: 1 addition & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sneaky!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could leave it there to always be green 😆


[testenv:check-build]
description = check build sdist/wheel and a strict twine check for metadata
Expand Down
Loading