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

[ruff] Implement compound comparison rule (RUF029) #10028

Closed

Conversation

tjkuson
Copy link
Contributor

@tjkuson tjkuson commented Feb 18, 2024

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
Copy link
Contributor Author

tjkuson commented Feb 18, 2024

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

@sbrugman
Copy link
Contributor

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

@tjkuson
Copy link
Contributor Author

tjkuson commented Feb 18, 2024

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

@Skylion007
Copy link
Contributor

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

@tjkuson
Copy link
Contributor Author

tjkuson commented Feb 22, 2024

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?

@Skylion007
Copy link
Contributor

Skylion007 commented Feb 22, 2024

Yeah, in RUFF it would be PLR1716 See #970

@tjkuson
Copy link
Contributor Author

tjkuson commented Feb 22, 2024

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?

@sbrugman
Copy link
Contributor

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.

@Skylion007
Copy link
Contributor

Ah you are right, my bad.

@MichaReiser
Copy link
Member

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?

@AlexWaygood
Copy link
Member

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.

@MichaReiser
Copy link
Member

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 MichaReiser closed this Apr 5, 2024
@tjkuson tjkuson deleted the non-transitive-compound-comparison branch April 5, 2024 15:07
@tjkuson
Copy link
Contributor Author

tjkuson commented Apr 5, 2024

@MichaReiser No worries!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule suggestion: lint against the use of in with compound comparisons
5 participants