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

[flake8-simplify] Implement SIM911 #9460

Merged
merged 6 commits into from
Jan 11, 2024

Conversation

trag1c
Copy link
Contributor

@trag1c trag1c commented Jan 10, 2024

Summary

Closes #9319, implements the SIM911 rule from flake8-simplify.

Note

I wasn't sure whether or not to include

if checker.settings.preview.is_disabled() {
    return;
}

at the beginning of the function with violation logic if the rule's already declared as part of RuleGroup::Preview.
I've seen both variants, so I'd appreciate some feedback on that :)

Copy link
Contributor

github-actions bot commented Jan 10, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Awesome, excited to review!

@charliermarsh charliermarsh self-requested a review January 11, 2024 03:12
@charliermarsh charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Jan 11, 2024
@charliermarsh
Copy link
Member

if checker.settings.preview.is_disabled() {
    return;
}

Ah yeah -- this shouldn't be required for rules that are in RuleGroup::Preview, only for logic within rules that varies based on whether preview is enabled. So omitting it here is correct in my view.

@trag1c
Copy link
Contributor Author

trag1c commented Jan 11, 2024

Thanks, corrected it. Double-checked my findings and it turns out that what I saw was actually a stable rule that had the preview check. I'm assuming that's still redundant?

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Looks great! Only minor feedback.

@charliermarsh
Copy link
Member

Thanks, corrected it. Double-checked my findings and it turns out that what I saw was actually a stable rule that had the preview check. I'm assuming that's still redundant?

Yeah, which rule is this?

@trag1c
Copy link
Contributor Author

trag1c commented Jan 11, 2024

Thanks, corrected it. Double-checked my findings and it turns out that what I saw was actually a stable rule that had the preview check. I'm assuming that's still redundant?

Yeah, which rule is this?

I checked to find the rule code only to realize it's just ONE OF the functions that has the preview check in which case it's probably fine 🤦‍♂️ forgive my silliness

@charliermarsh
Copy link
Member

@trag1c - I'm happy to include this in today's release. Do you wanna do the follow-up feedback otherwise I can do it quickly (no bother either way)?

@trag1c
Copy link
Contributor Author

trag1c commented Jan 11, 2024

Sure, let's get that released today! 😄

Copy link

codspeed-hq bot commented Jan 11, 2024

CodSpeed Performance Report

Merging #9460 will degrade performances by 4.35%

Comparing trag1c:simplify-SIM911 (ea5a54a) with main (f192c72)

Summary

❌ 1 regressions
✅ 29 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main trag1c:simplify-SIM911 Change
parser[numpy/ctypeslib.py] 12.2 ms 12.8 ms -4.35%

@charliermarsh
Copy link
Member

@trag1c - think it just needs to be updated against main, just pushed.

@charliermarsh charliermarsh enabled auto-merge (squash) January 11, 2024 19:32
auto-merge was automatically disabled January 11, 2024 19:33

Head branch was pushed to by a user without write access

@trag1c
Copy link
Contributor Author

trag1c commented Jan 11, 2024

My brain ignored "just pushed" and decided to update it myself either way 🤦‍♂️
Sorry for messing up again

@charliermarsh charliermarsh merged commit eb4ed24 into astral-sh:main Jan 11, 2024
16 of 17 checks passed
@charliermarsh
Copy link
Member

(This doesn't touch the parser at all so it must be noise.)

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.

Implement new SIM rule SIM911: zip(dict.keys(), dict.values()) → dict.items()
2 participants