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

[refurb] New rule to suggest min/max over sorted() (FURB192) #10868

Merged
merged 32 commits into from Apr 23, 2024

Conversation

ottaviohartman
Copy link
Contributor

@ottaviohartman ottaviohartman commented Apr 11, 2024

Summary

Fixes #10463

Add FURB192 which detects violations like this:

# Bad
a = sorted(l)[0]

# Good
a = min(l)

There is a caveat that @Skylion007 has pointed out, which is that violations with reverse=True technically aren't compatible with this change, in the edge case where the unstable behavior is intended. For example:

from operator import itemgetter
data = [('red', 1), ('blue', 1), ('red', 2), ('blue', 2)]

min(data, key=itemgetter(0))  # ('blue', 1)
sorted(data, key=itemgetter(0))[0]  # ('blue', 1)
sorted(data, key=itemgetter(0), reverse=True)[-1]  # ('blue, 2')

This seems like a rare edge case, but I can make the reverse=True fixes unsafe if that's best.

Test Plan

This is unit tested.

References

https://github.com/dosisod/refurb/pull/333/files

@@ -3075,6 +3075,8 @@
"FURB180",
"FURB181",
"FURB187",
"FURB19",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This auto-generated FURB19 but I can't figure out why

Copy link
Contributor

github-actions bot commented Apr 11, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

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

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

+ airflow/providers/yandex/secrets/lockbox.py:258:16: FURB192 [*] Prefer `min` over `sorted()` to compute the minimum value in a sequence

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

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

+ rotkehlchen/db/dbhandler.py:269:25: FURB192 [*] Prefer `max` over `sorted()` to compute the maximum value in a sequence
+ rotkehlchen/globaldb/handler.py:127:21: FURB192 [*] Prefer `max` over `sorted()` to compute the maximum value in a sequence

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

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

+ indico/modules/events/registration/util.py:272:16: FURB192 [*] Prefer `min` over `sorted()` to compute the minimum value in a sequence

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
FURB192 4 4 0 0 0

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule accepted Ready for implementation labels Apr 11, 2024
@Skylion007
Copy link

Skylion007 commented Apr 12, 2024

I'd definitely advocate for making unstable sort fixes unsafe. Otherwise, it could unknowingly change user code. I'd also argue we may want add to add a config option for this rule that ignores unstable sorts.


let replacement = if let Some(key) = &key_keyword_expr {
format!(
"{}({}, key={})",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a cleaner or more canonical way to generate this?

@ottaviohartman
Copy link
Contributor Author

@Skylion007

I'd definitely advocate for making unstable sort fixes unsafe. Otherwise, it could unknowingly change user code.

Agreed, and updated!

I'd also argue we may want add to add a config option for this rule that ignores unstable sorts.

Shall I do this too? Haven't heard anyone else chime in yet.

Copy link

codspeed-hq bot commented Apr 16, 2024

CodSpeed Performance Report

Merging #10868 will not alter performance

Comparing ottaviohartman:thartman/furb192 (da36bfc) with main (925c7f8)

Summary

✅ 30 untouched benchmarks

@zanieb
Copy link
Member

zanieb commented Apr 16, 2024

Shall I do this too? Haven't heard anyone else chime in yet.

I probably wouldn't implement it now since the rule is just going into preview. We can use preview feedback to consider adding an option. I'd just open a tracking issue when this is merged.

@charliermarsh charliermarsh self-assigned this Apr 23, 2024
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.

Thanks!

@charliermarsh charliermarsh enabled auto-merge (squash) April 23, 2024 01:09
@charliermarsh charliermarsh merged commit 111bbc6 into astral-sh:main Apr 23, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule Suggestion min/max instead of sorted(l)[0]
6 participants