[ruff] Abstract methods defined in normal classes (RUF044)#14688
[ruff] Abstract methods defined in normal classes (RUF044)#14688InSyncWithFoo wants to merge 12 commits intoastral-sh:mainfrom
ruff] Abstract methods defined in normal classes (RUF044)#14688Conversation
ce600ca to
067d25e
Compare
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF044 | 102 | 102 | 0 | 0 | 0 |
81d34f6 to
eee7e9f
Compare
crates/ruff_linter/src/rules/ruff/rules/abstract_method_in_normal_class.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/abstract_method_in_normal_class.rs
Outdated
Show resolved
Hide resolved
3cd3f17 to
cdd3f4c
Compare
crates/ruff_linter/src/rules/ruff/rules/abstract_method_in_normal_class.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/abstract_method_in_normal_class.rs
Outdated
Show resolved
Hide resolved
|
@MichaReiser The function cannot be called |
|
@InSyncWithFoo what's an example where if there are indeed cases where some of those methods return false when the class isn't abstract, then it's important that we document these as part of the rule. |
|
A few examples: from abc import ABCMeta
from somewhere import SomeMetaclass # Could be subclass of ABCMeta, for all we know.
# `B` might be abstract (methods are not checked).
class A(ABCMeta): ...
class B(metaclass=A): ...
# Is `C` abstract or not?
class C(metaclass=SomeMetaclass): ...
# `D` and `E` definitely aren't abstract.
class D: ...
class E(D): ... |
|
Thanks for the example. I feel like we can keep using
|
6515be2 to
85e0bbf
Compare
|
Here's a table:
|
|
Thanks for the table. I see. I'm then leaning towards simply calling it I also noticed that we have ruff/crates/ruff_linter/src/rules/flake8_bugbear/rules/abstract_base_class.rs Lines 116 to 129 in 14ba469 |
|
@MichaReiser // This doesn't read well in English: "If *not* `class` is *not* abstract, bail."
if !is_not_abstract(class, checker.semantic()) {
return;
}
// This reads better: "If `class` might be abstract, bail."
if might_be_abstract(class, checker.semantic()) {
return;
}As for |
That's fair. My only concern is that
I'm not sure if we want to update |
Visualization: Are you sure that you want |
|
To me the two rules must be consistent in their behavior. That means they should both respect Whether both rules use the exact same code is less important but it would be nice if they can share most of the: Does this class implement |
|
That
|
MichaReiser
left a comment
There was a problem hiding this comment.
My main concern is that this PR adds more code to identify abstract classes that are hidden away in the rule implementation and isn't shared with the logic that other rules use. I'm okay looking over the ABCMeta difference if we plan on addressing that later, as long as my main concern is addressed
85e0bbf to
a2e2aa7
Compare
06db816 to
698cbe6
Compare
ruff] Abstract method defined in normal class (RUF044)ruff] Abstract methods defined in normal class (RUF044)
ruff] Abstract methods defined in normal class (RUF044)ruff] Abstract methods defined in normal classes (RUF044)
|
This is ready for a re-review now. |
|
Did this ever get implemented "Abstract methods defined in normal classes (RUF044)" or are there plans to? https://docs.astral.sh/ruff/rules/#ruff-specific-rules-ruf |
|
@InSyncWithFoo what is the state of this feature? I find it useful and ready to help if needed. |
|
@amaslenn This PR is outdated due to not having a reviewer. I'm willing to update it as long as there's one. |
@MichaReiser could you please review this PR? |
Summary
Resolves #12861.
This change introduces
RUF044along with a new shared logic for detecting whether a given class is abstract or not. The details on how that works can be found in the doc comments.Test Plan
cargo nextest runandcargo insta test.