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

[pyupgrade] Replace str,Enum with StrEnum UP042 #10713

Merged
merged 6 commits into from
Apr 6, 2024

Conversation

Warchant
Copy link
Contributor

@Warchant Warchant commented Apr 1, 2024

Disclaimer: I am first time contributor to Ruff, relatively new to Rust. Doing this to learn the language, study how ruff project is structured and to pay back for using my favourite python tool :)

Summary

Add new rule pyupgrade - UP042 (I picked next available number).
Closes #3867
Closes #9569

It should warn + provide a fix class A(str, Enum) -> class A(StrEnum) for py311+.

Test Plan

Added UP042.py test.

Notes

I did not find a way to call remove_argument 2 times consecutively, so the automatic fixing works only for classes that inherit exactly str, Enum (regardless of the order).

I also plan to extend this rule to support IntEnum in next PR.

Copy link

codspeed-hq bot commented Apr 1, 2024

CodSpeed Performance Report

Merging #10713 will not alter performance

Comparing Warchant:main (d677f25) with main (323264d)

Summary

✅ 30 untouched benchmarks

@Warchant Warchant force-pushed the main branch 4 times, most recently from cea7a40 to 3559ba3 Compare April 1, 2024 20:34
@Warchant Warchant marked this pull request as ready for review April 1, 2024 20:45
Copy link
Contributor

github-actions bot commented Apr 1, 2024

ruff-ecosystem results

Linter (stable)

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

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

- indico/modules/admin/notices.py:40:1: UP042 Class NoticeSeverity inherits from both `str` and `enum.Enum`. Prefer `enum.StrEnum` instead.

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
UP042 1 0 1 0 0

Linter (preview)

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

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

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

- indico/modules/admin/notices.py:40:1: UP042 Class NoticeSeverity inherits from both `str` and `enum.Enum`. Prefer `enum.StrEnum` instead.
+ indico/modules/admin/notices.py:40:7: UP042 Class NoticeSeverity inherits from both `str` and `enum.Enum`

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
UP042 2 1 1 0 0

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Apr 2, 2024
@Warchant
Copy link
Contributor Author

Warchant commented Apr 5, 2024

Anyone?

@charliermarsh
Copy link
Member

I’ll take a look today :)

@charliermarsh charliermarsh self-assigned this Apr 5, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) April 6, 2024 01:42
@charliermarsh
Copy link
Member

This is great! I made some changes to the documentation and rule messages just to better align with our style elsewhere.

let mut inherits_enum = false;
for base in arguments.args.iter() {
if let Some(qualified_name) = checker.semantic().resolve_qualified_name(base) {
match qualified_name.segments() {
Copy link
Member

Choose a reason for hiding this comment

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

I changed this to a match over the segments, rather than an if-else.

#[derive_message_formats]
fn message(&self) -> String {
let ReplaceStrEnum { name, .. } = self;
format!("Class {name} inherits from both `str` and `enum.Enum`")
Copy link
Member

Choose a reason for hiding this comment

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

We prefer to omit the fix in the rule message.

ReplaceStrEnum {
name: class_def.name.to_string(),
},
class_def.identifier(),
Copy link
Member

Choose a reason for hiding this comment

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

The use of .identifier() here is nice because it confines the range of the diagnostic to the class name, rather than the entire class definition (which would include the class body).

class D(int, str, Enum): ...


class E(str, int, Enum): ...
Copy link
Member

Choose a reason for hiding this comment

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

(I ran ruff format over the fixture. It's not a hard rule, since fixtures are often intentionally unformatted, but it's nice when we don't rely on the formatting anywhere.)