-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
] Empty branches (RUF050
)
#14763
Conversation
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
RUF050 | 286 | 286 | 0 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
Currently this rule is a bit too overzealous. It will also flag empty Personally, I'm in favor of keeping this behaviour, as it enforces refactoring/denesting. I understand that I'm exceedingly likely to be in the minority, however. |
From a first skim through the ecosystem checks: We should exclude branches with comments because it expresses that the branch is intentional:
I know this is quiet the opposite of what I said in the issue, but I just realised that clippy intentionally has multiple rules:
Thinking this through more, this makes sense to me because a needless-if is just useless where an empty loop can be correct sometimes.
I like using |
In such cases, I would move the conditions to another function to take advantage of early returns, but that's just me. As for splitting, I would like to use and reserve the |
That's fair. I just think it's too opinionated for this rule. Such a branch isn't needless; it's more of a stylistic choice; you don't want any empty branches.
I don't think we want to block off an entire range for just a few rules. Let's also hear from @AlexWaygood to ensure we're aligned to avoid changing our approach mroe than once |
I agree. My intention when I opened the issue was just to detect what is most likely unintentional dead code (e.g., empty conditionals from other auto-fixes/formatters). I would not want deliberate style choices to get flagged, especially the empty branches with comments. |
Thanks for keeping this PR up to date. I talked to @AlexWaygood, and we agree that we should split this rule into multiple smaller rules. I'll comment on the issue with a suggestion. I'm sorry to have lead you in the wrong direction :( |
Summary
Resolves #13929 and #9472.
Test Plan
cargo nextest run
andcargo insta test
.