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.

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

Ruff fixes: D402, D400 , D209 , D202 , B007 , I001 , B018 #103

Merged
merged 7 commits into from
Feb 1, 2025

Conversation

jo47011
Copy link
Contributor

@jo47011 jo47011 commented Jan 24, 2025

Removed some of the ruff.lint exceptions from pyproject.toml.

I did a commit for each line removed. Thus you can easily review them step-by-step by looking at the individual commits.

There are still quite some exception most of them are documentation related and will be quite some work to fix. Others need some more discussion on our code preferences and/or a more in-depth knowledge of the code.

So we could somehow discuss them step-by-step or maybe just leave them there for now. Maybe you have a proposal on how we shall address it?

Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 97.22222% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.25%. Comparing base (9231782) to head (8b19a72).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
shc/interfaces/pulse.py 85.71% 1 Missing ⚠️
shc/util/check_shc.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
- Coverage   83.32%   83.25%   -0.07%     
==========================================
  Files          42       42              
  Lines        5636     5631       -5     
  Branches      768      768              
==========================================
- Hits         4696     4688       -8     
- Misses        739      741       +2     
- Partials      201      202       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mhthies
Copy link
Owner

mhthies commented Feb 1, 2025

Regarding the remaining ignored Ruff rules:

"B006", # Do not use mutable data structures for argument defaults
"B012", # return inside finally blocks cause exceptions to be silenced

I assume that violations of these rules can either be fixed or explicitly ignored (including a descriptive comment, why violating the rule is reasonable in the specific case).

"B023", # Function definition does not bind loop variable spec

Oh, this is an important one, to find ugly bugs. I would definitely enable this one and ignore false-positives on a per-line basis, if required.

"B024", # abstract base class, but it has no abstract methods or properties

Not sure about this one. There may be multiple violations of this one in SHC, caused by the trait-/mixin-style class inheritance patterns. My intuition tells me that we should keep this one ignored, as I don't see a real problem with violations.

"B027", # empty method in an abstract base class, but has no abstract decorator

This seems a bit over-pedantic. Why shouldn't the abstract base class provide a default implementation which is a noop?
Still, I would be fine with ignoring it explicitly in the code at every occurrence.

"B028", # No explicit stacklevel keyword argument found

This rule sounds very reasonable. I would really want to fix violations of this one.

"B904", # Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

I like this one. There may be rare cases, where an Exception in an except clause is actually meant to report an error during exception handling, but in these cases, I think, it's reasonable to ignore the rule locally for the relevant line.

"D100", # Missing docstring in public module
"D101", # Missing docstring in public class
"D102", # Missing docstring in public method
"D103", # Missing docstring in public function
"D104", # Missing docstring in public package
"D105", # Missing docstring in magic method
"D106", # Missing docstring in public nested class
"D107", # Missing docstring in public init

I would keep these disabled. I'm a fan of naming functions/methods and parameters in a way that is self-explanatory (together with type-hints) and does not require an additional docstring (which would only repeat, what the denominators and type hints already say).

"D203", # Ignore one-blank-line-before-class as Black handles this

Sounds reasonable to keep this ignored.

"D200", # One-line docstring should fit on one line
"D213", # Multi-line docstring summary should start at the second line
"D204", # 1 blank line required after class docstring

Those sound good for a consistent layout of docstrings.

"D212", # Multi-line docstring summary should start at the first line

This conflicts with D213 and I like D213 better.

"D301", # Use r""" if any backslashes in a docstring
"D401", # First line of docstring should be in imperative mood
"D404", # First word of the docstring should not be "This"

I'm unsure about these. All of these sound like unnecessary constraints to me.

"D205", # 1 blank line required between summary line and description

I assume, this conflicts with a two-line summary at the beginning of the docstring. I find, sometimes two lines are required for a good summary, so I don't want to be limited by this rule. Thus, I would keep it ignored.

@mhthies mhthies merged commit 40fb62d into mhthies:main Feb 1, 2025
9 checks passed
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.

2 participants