[`flake8-simplify`] Implement `SIM911` by trag1c · Pull Request #9460 · astral-sh/ruff

trag1c · 2024-01-10T23:14:14Z

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

github-actions · 2024-01-10T23:27:14Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

charliermarsh

Awesome, excited to review!

charliermarsh · 2024-01-11T03:13:09Z

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 · 2024-01-11T10:04:27Z

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?

crates/ruff_linter/src/rules/flake8_simplify/rules/zip_dict_keys_and_values.rs

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

charliermarsh

Looks great! Only minor feedback.

charliermarsh · 2024-01-11T18:47:09Z

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 · 2024-01-11T18:55:03Z

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 · 2024-01-11T19:00:05Z

@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 · 2024-01-11T19:05:04Z

Sure, let's get that released today! 😄

codspeed-hq · 2024-01-11T19:10:27Z

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 · 2024-01-11T19:32:32Z

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

trag1c · 2024-01-11T19:36:02Z

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

charliermarsh · 2024-01-11T19:42:54Z

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