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.)

@charliermarsh charliermarsh added the preview Related to preview mode features label Apr 6, 2024
@charliermarsh
Copy link
Member

Left a few clarifying comments on changes that I made, but please ask questions if you'd like.

@charliermarsh charliermarsh merged commit b45fd61 into astral-sh:main Apr 6, 2024
17 checks passed
Glyphack pushed a commit to Glyphack/ruff that referenced this pull request Apr 12, 2024
…#10713)

## Summary

Add new rule `pyupgrade - UP042` (I picked next available number).
Closes astral-sh#3867
Closes astral-sh#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pyupgrade rule: upgrading str, Enum to StrEnum
3 participants