Skip to content

[refurb] Implement subclass-builtin (FURB189)#14105

Merged
dhruvmanila merged 6 commits intoastral-sh:mainfrom
sbrugman:refurb-no-subclass-builtins
Nov 7, 2024
Merged

[refurb] Implement subclass-builtin (FURB189)#14105
dhruvmanila merged 6 commits intoastral-sh:mainfrom
sbrugman:refurb-no-subclass-builtins

Conversation

@sbrugman
Copy link
Copy Markdown
Contributor

@sbrugman sbrugman commented Nov 5, 2024

Summary

Implementation for one of the rules in #1348
Refurb only deals only with classes with a single base, however the rule is valid for any base.
(str, Enum is common prior to StrEnum)

Test Plan

cargo test

@sbrugman sbrugman force-pushed the refurb-no-subclass-builtins branch 3 times, most recently from 4da72a1 to 0f22b54 Compare November 5, 2024 12:35
@sbrugman sbrugman force-pushed the refurb-no-subclass-builtins branch from 0f22b54 to 2f63930 Compare November 5, 2024 12:36
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 5, 2024 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+12 -0 violations, +0 -0 fixes in 5 projects; 49 projects unchanged)

apache/airflow (+6 -0 violations, +0 -0 fixes) 

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/decorators/setup_teardown.py:58:22: FURB189 Subclassing `list` can be error prone, use `collections.UserList` instead
+ airflow/io/utils/stat.py:22:19: FURB189 Subclassing `dict` can be error prone, use `collections.UserDict` instead
+ kubernetes_tests/test_base.py:45:26: FURB189 Subclassing `str` can be error prone, use `collections.UserStr` instead
+ providers/src/airflow/providers/openlineage/utils/utils.py:170:25: FURB189 Subclassing `dict` can be error prone, use `collections.UserDict` instead
+ providers/tests/openlineage/plugins/test_utils.py:67:19: FURB189 Subclassing `dict` can be error prone, use `collections.UserDict` instead
+ tests/utils/log/test_secrets_masker.py:317:54: FURB189 Subclassing `str` can be error prone, use `collections.UserStr` instead

bokeh/bokeh (+1 -0 violations, +0 -0 fixes) 

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ src/bokeh/models/plots.py:903:24: FURB189 Subclassing `list` can be error prone, use `collections.UserList` instead

langchain-ai/langchain (+1 -0 violations, +0 -0 fixes) 

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ libs/core/tests/unit_tests/stubs.py:7:14: FURB189 Subclassing `str` can be error prone, use `collections.UserStr` instead

scikit-build/scikit-build-core (+2 -0 violations, +0 -0 fixes) 

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ src/scikit_build_core/settings/skbuild_model.py:31:27: FURB189 Subclassing `str` can be error prone, use `collections.UserStr` instead
+ src/scikit_build_core/setuptools/build_cmake.py:214:24: FURB189 Subclassing `list` can be error prone, use `collections.UserList` instead

indico/indico (+2 -0 violations, +0 -0 fixes) 

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ indico/legacy/pdfinterface/latex.py:169:16: FURB189 Subclassing `str` can be error prone, use `collections.UserStr` instead
+ indico/modules/events/abstracts/util.py:141:24: FURB189 Subclassing `dict` can be error prone, use `collections.UserDict` instead

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
FURB189 12 12 0 0 0

dhruvmanila
dhruvmanila requested changes Nov 6, 2024
View reviewed changes
Comment thread crates/ruff_linter/src/rules/refurb/rules/subclass_builtin.rs Outdated
Comment thread crates/ruff_linter/src/rules/refurb/rules/subclass_builtin.rs Outdated
Comment thread crates/ruff_linter/src/rules/refurb/rules/subclass_builtin.rs Outdated
Comment thread crates/ruff_linter/src/rules/refurb/rules/subclass_builtin.rs Outdated
Comment thread crates/ruff_linter/src/rules/refurb/rules/subclass_builtin.rs Outdated
Comment thread crates/ruff_linter/src/rules/refurb/rules/subclass_builtin.rs Outdated
@dhruvmanila dhruvmanila added rule Implementing or modifying a lint rule preview Related to preview mode features labels Nov 6, 2024
@sbrugman sbrugman force-pushed the refurb-no-subclass-builtins branch from 56cc5ea to c7f45b0 Compare November 6, 2024 15:52
@sbrugman
Copy link
Copy Markdown
Contributor Author

sbrugman commented Nov 6, 2024

Thanks for the helpful pointers @dhruvmanila!

@dhruvmanila
Minor tweaks
908fb63 
- Move utilities after rule logic
- Avoid allocating string multiple times
dhruvmanila
dhruvmanila approved these changes Nov 7, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Thanks! I made a couple of minor changes:

  • Moved the utilities below the rule logic function; we do this for all rules
  • Avoid the eager .to_string call for user_symbol method and only allocate it when creating the Diagnostic

@dhruvmanila
Copy link
Copy Markdown
Member

dhruvmanila commented Nov 7, 2024 •
edited
Loading

Looking at the ecosystem changes, we might want to special case (str, Enum).

Refurb only deals only with classes with a single base, however the rule is valid for any base.

Sorry, I missed this earlier but can you expand on why do we want to diverge from this behavior.

Edit: I think we should keep the same behavior as refurb and check only when there's one base class.

@sbrugman
Copy link
Copy Markdown
Contributor Author

sbrugman commented Nov 7, 2024

Fair point. After looking at the ecosystem results, I think we indeed should stick with refurb's behaviour (at least until type inference is available).

Only considering classes with a single base is the least error prone.

It also has some false negatives which can be detected by considering multiple bases, e.g. https://github.com/bokeh/bokeh/blob/829b2a75c402d0d0abd7e37ff201fbdfd949d857/src/bokeh/core/property/wrappers.py#L306.

However there are false positives for:

dhruvmanila
dhruvmanila reviewed Nov 7, 2024
View reviewed changes
Comment thread crates/ruff_linter/src/rules/refurb/rules/subclass_builtin.rs Outdated
@dhruvmanila
Copy link
Copy Markdown
Member

dhruvmanila commented Nov 7, 2024

It also has some false negatives which can be detected by considering multiple bases, e.g. bokeh/bokeh@829b2a7/src/bokeh/core/property/wrappers.py#L306.

However there are false positives for:

I think the listed limitations are a good trade-off.

@dhruvmanila dhruvmanila changed the title [refurb] Implement subclass-builtin (FURB189) [refurb] Implement subclass-builtin (FURB189) Nov 7, 2024
@dhruvmanila dhruvmanila merged commit 136721e into astral-sh:main Nov 7, 2024
@sbrugman sbrugman deleted the refurb-no-subclass-builtins branch November 7, 2024 11:59
@BrewTestBot BrewTestBot mentioned this pull request Nov 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhruvmanila dhruvmanila dhruvmanila approved these changes

Assignees

No one assigned

Labels

preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sbrugman @dhruvmanila