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

Reduce ruff warnings #778

Merged
merged 4 commits into from
Mar 31, 2025
Merged

Reduce ruff warnings #778

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7cc68ca
reduce ruff warnings:
danielsparing Mar 28, 2025
143c3ac
end-of-file-fixer
danielsparing Mar 28, 2025
df5c9b9
Revert "end-of-file-fixer"
danielsparing Mar 28, 2025
9f359f5
Merge branch 'main' into reduce-ruff-warnings
kylebarron Mar 30, 2025
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
4 changes: 3 additions & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ ignore = [
"B028", # No explicit `stacklevel` keyword argument found
"B011", # Do not `assert False` (`python -O` removes these calls), raise `AssertionError()`
"D100", # Missing docstring in public module
"D203", # incorrect-blank-line-before-class
"D213", # multi-line-summary-second-line
Comment on lines +122 to +123
Copy link
Member

Choose a reason for hiding this comment

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

Were there previously errors that these were catching? Main is clean in terms of lints?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So these two were already ignored, because they are conficting two other rules (and hence would catch "errors" if they were active), as the below warning within the CI runs states (e.g. here):

  - 'extend-per-file-ignores' -> 'lint.extend-per-file-ignores'
warning: `incorrect-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `incorrect-blank-line-before-class`.
warning: `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Ignoring `multi-line-summary-second-line`.

Therefore it comes down to your preference:

  • Do you want an extra newline between class definition and docstring? (D203 vs D211)
  • Within the docstring, do you want the summary line to start on the first or second line? (D212 vs D213)

In this commit I made the choices so that the existing codebase still passes. If you were to invert either or both of them, then that would require an (automatic) edit all the docstrings elsewhere too.

Copy link
Member

Choose a reason for hiding this comment

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

Oh... I see.

  • Prefer no extra newline between class def and docstring
  • Prefer summary line on the first line (d212)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I think it's all OK:

  • the repo is already following these preferences
  • D203 and D213 would have gone against these preferences -- but they were already ignored as they were conflicting with their opposite rules -- but now with this PR, there won't even be a warning emitted about this clash.

"D406", # Section name should end with a newline ("Returns")Ruff
"D407", # Missing dashed underline after section
"E501", # Line too long
Expand Down Expand Up @@ -161,7 +163,7 @@ ignore = [
"SLF001", # Private member accessed
]

[tool.ruff.extend-per-file-ignores]
[tool.ruff.lint.extend-per-file-ignores]
"__init__.py" = [
"F401", # Allow unused imports in __init__.py files
]
Expand Down