ruff icon indicating copy to clipboard operation
ruff copied to clipboard

[`ruff`] Implement compound comparison rule (`RUF029`)

Open tjkuson opened this issue 1 year ago • 8 comments

Summary

Implements non-transitive-compound-comparison (RUF029) that disallows certain compound comparisons.

As suggested,

Arguably we should lint against all compound Compares except the following:

* Both compares are `is`: `a is b is c`

* Both compares are `==`: `a == b ==c`

* Both compares are one of `<`, `<=`: `a <= b < c` (probably the most common use  of compound Compare)

* Both compares are one of `>`, `>=`: `a >= b > c`

Not a huge fan of the name I gave it, though. Chose RUF029 as RUF028 has been used in a bunch of open PRs.

Closes #8964.

Test Plan

cargo nextest run

tjkuson avatar Feb 18 '24 15:02 tjkuson

I don't think the ecosystem CI failure is to do with me...

tjkuson avatar Feb 18 '24 16:02 tjkuson

This pylint rule is related (to the introduction mostly): https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/chained-comparison.html

Perhaps you'd be interested to implement it too

sbrugman avatar Feb 18 '24 22:02 sbrugman

Huh, I thought that was already implemented in Ruff. Thanks for the idea @sbrugman, I'll implement it tomorrow.

tjkuson avatar Feb 18 '24 23:02 tjkuson

Make sense for this to be a pylint rule more than a RUF rule.

Skylion007 avatar Feb 21 '24 21:02 Skylion007

Make sense for this to be a pylint rule more than a RUF rule.

I am not aware of there being a corresponding Pylint rule on which to map, is there one?

tjkuson avatar Feb 22 '24 09:02 tjkuson

Yeah, in RUFF it would be PLR1716 See https://github.com/astral-sh/ruff/issues/970

Skylion007 avatar Feb 22 '24 22:02 Skylion007

I might be misunderstanding something, sorry. That turns things into compound comparisons (have a PR for that tomorrow). This rule does the opposite. They should be separate rules, no?

tjkuson avatar Feb 22 '24 22:02 tjkuson

Indeed, I understand it the same way. The current PR implements a new rule that does not exist in pylint (although it would be a good fit there) and thus should be assigned the latest available RUFF code, e.g. RUF029.

The PLR1716 is the linked related rule that is included by Pylint, but not implemented in the PR.

sbrugman avatar Feb 22 '24 22:02 sbrugman

Ah you are right, my bad.

Skylion007 avatar Feb 23 '24 01:02 Skylion007

I'm undecided how we should categorize this rule. To me it's a restriction rule because it disallows a subset of Python's grammar. However, I could also see that the rule fits into suspicious because the code probably doesn't do what you intended. @AlexWaygood what do you think?

MichaReiser avatar Apr 05 '24 10:04 MichaReiser

I think it's a useful rule to enforce a Python style that's more readable, but I'd certainly agree that it should be disabled by default.

AlexWaygood avatar Apr 05 '24 10:04 AlexWaygood

Thank you @tjkuson for working on this rule and sorry for the late feedback.

We agree that the rule itself is valuable and think it should be added to Ruff eventually. But we don't feel comfortable doing it today before #1774 is merged. The rule restricts the allowed Python syntax in an opinionated way, which we aren't comfortable enabling by default but all users that use RUFF or ALL today would automatically opt in to the rule.

That's why we want to hold back with the rule for now but hope to get back to it after #1774 is completed. Sorry that we only decided this after you implemented the rule (without mentioning on the issue)

MichaReiser avatar Apr 05 '24 13:04 MichaReiser

@MichaReiser No worries!

tjkuson avatar Apr 05 '24 15:04 tjkuson