Skip to content

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

Merged
charliermarsh merged 32 commits intoastral-sh:mainfrom
ottaviohartman:thartman/furb192
Apr 23, 2024
Merged

[refurb] New rule to suggest min/max over sorted() (FURB192)#10868
charliermarsh merged 32 commits intoastral-sh:mainfrom
ottaviohartman:thartman/furb192

Conversation

@ottaviohartman
Copy link
Contributor

@ottaviohartman ottaviohartman commented Apr 11, 2024 •
edited
Loading

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

ottaviohartman
ottaviohartman commented Apr 11, 2024
View reviewed changes
ruff.schema.json
"FURB180",
"FURB181",
"FURB187",
"FURB19",
Copy link
Contributor Author

@ottaviohartman ottaviohartman Apr 11, 2024

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

ottaviohartman
ottaviohartman commented Apr 11, 2024
View reviewed changes
@github-actions
Copy link
Contributor

github-actions bot commented Apr 11, 2024 •
edited
Loading

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
VascoSch92
VascoSch92 reviewed Apr 11, 2024
View reviewed changes
@Skylion007
Copy link
Contributor

Skylion007 commented Apr 12, 2024 •
edited
Loading

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.

ottaviohartman
ottaviohartman commented Apr 16, 2024
View reviewed changes
crates/ruff_linter/src/rules/refurb/rules/sorted_min_max.rs Outdated

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

@ottaviohartman ottaviohartman Apr 16, 2024

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
ottaviohartman commented Apr 16, 2024
View reviewed changes
@ottaviohartman
Copy link
Contributor Author

ottaviohartman commented Apr 16, 2024

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

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 16, 2024 •
edited
Loading

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
charliermarsh
charliermarsh approved these changes Apr 23, 2024
View reviewed 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.

Thanks!

@charliermarsh charliermarsh enabled auto-merge (squash) April 23, 2024 01:09
@charliermarsh charliermarsh merged commit 111bbc6 into astral-sh:main Apr 23, 2024
@BrewTestBot BrewTestBot mentioned this pull request Apr 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser left review comments

@zanieb zanieb zanieb left review comments

@charliermarsh charliermarsh charliermarsh approved these changes

@VascoSch92 VascoSch92 Awaiting requested review from VascoSch92

Assignees

@charliermarsh charliermarsh

Labels

accepted Ready for implementation rule Implementing or modifying a lint rule

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

6 participants

@ottaviohartman @Skylion007 @zanieb @MichaReiser @charliermarsh @VascoSch92