-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add rule removal infrastructure #9691
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's nice!
Although, we should also remove the files and tests for the rule. And we should probably ensure it doesn't end up in the docs? |
fn parse_and_or_ternary(bool_op: &ExprBoolOp) -> Option<(&Expr, &Expr, &Expr)> { | ||
if bool_op.op != BoolOp::Or { | ||
return None; | ||
unreachable!("PLR1706 has been removed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get rid of more here? I'm not sure.
@@ -26,6 +27,9 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator<Item = Rule>, | |||
table_out.push('\n'); | |||
for rule in rules { | |||
let status_token = match rule.group() { | |||
RuleGroup::Removed => { | |||
format!("<span title='Rule has been removed'>{REMOVED_SYMBOL}</span>") | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My vote would be to remove rules from this table entirely. What's the motivation for keeping them around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's another way to navigate to them. The motivation for keeping them is to
- Document why they were removed
- Provide a reference for people on previous versions of Ruff
I don't think I expect to keep them around forever; this was an easy path forward though. When we do rule recategorization (#1774), I think we should look at this again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems fair to me to list the rules here when we document the removal on the details page. I do see this as obsolete if we have a versioned documentation.
You may want to consider listing these rules last and designing them visually less prominent (e.g., graying them out).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to gray them out but it actually seems like a huge pain to do it inside the markdown table. I think this will become obsolete when we do more work on the documentation but I do not think now is the time to try to build that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add two screenshots of the documentation to your test plan:
- showing the generated rules table
- the details page
@@ -26,6 +27,9 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator<Item = Rule>, | |||
table_out.push('\n'); | |||
for rule in rules { | |||
let status_token = match rule.group() { | |||
RuleGroup::Removed => { | |||
format!("<span title='Rule has been removed'>{REMOVED_SYMBOL}</span>") | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems fair to me to list the rules here when we document the removal on the details page. I do see this as obsolete if we have a versioned documentation.
You may want to consider listing these rules last and designing them visually less prominent (e.g., graying them out).
} | ||
|
||
fn fix_title(&self) -> Option<String> { | ||
Some(format!("Convert to if-else expression")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember that this needs to be a format!
call because we extract it somewhere (or was it message). @charliermarsh do you remember the details? Do we need to preserve the message
and fix_title
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be a format!
call for the derive_message_formats
macro but I've hard-coded the message formats.
@MichaReiser added those — I'm not in love with them but I think they're a good incremental step forward. |
Thank you @zanieb. What do you think of crossing out the rule names for rules that have been removed? The icon is very distant and hard to spot. E.g. |
@MichaReiser I guess that's an option. Could I populate the message section instead? e.g. "This rule has been removed"? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichaReiser I guess that's an option. Could I populate the message section instead? e.g. "This rule has been removed"?
My preferred solution would be to have both but we can iterate on this later.
Removes PLR1706 for testing
Similar to #9689 — retains removed rules for better error messages and documentation but removed rules _cannot_ be used in any context. Removes PLR1706 as a useful test case and something we want to accomplish in #9680 anyway. The rule was in preview so we do not need to deprecate it first. Closes #9007 ## Test plan <img width="1110" alt="Rules table" src="https://github.com/astral-sh/ruff/assets/2586601/ac9fa682-623c-44aa-8e51-d8ab0d308355"> <img width="1110" alt="Rule page" src="https://github.com/astral-sh/ruff/assets/2586601/05850b2d-7ca5-49bb-8df8-bb931bab25cd">
Updated implementation of #7369 which was left out in the cold. This was motivated again following changes in #9691 and #9689 where we could not test the changes without actually deprecating or removing rules. --- Follow-up to discussion in #7210 Moves integration tests from using rules that are transitively in nursery / preview groups to dedicated test rules that only exist during development. These rules always raise violations (they do not require specific file behavior). The rules are not available in production or in the documentation. Uses features instead of `cfg(test)` for cross-crate support per rust-lang/cargo#8379
Similar to #9689 — retains removed rules for better error messages and documentation but removed rules _cannot_ be used in any context. Removes PLR1706 as a useful test case and something we want to accomplish in #9680 anyway. The rule was in preview so we do not need to deprecate it first. Closes #9007 ## Test plan <img width="1110" alt="Rules table" src="https://github.com/astral-sh/ruff/assets/2586601/ac9fa682-623c-44aa-8e51-d8ab0d308355"> <img width="1110" alt="Rule page" src="https://github.com/astral-sh/ruff/assets/2586601/05850b2d-7ca5-49bb-8df8-bb931bab25cd">
Similar to #9689 — retains removed rules for better error messages and documentation but removed rules _cannot_ be used in any context. Removes PLR1706 as a useful test case and something we want to accomplish in #9680 anyway. The rule was in preview so we do not need to deprecate it first. Closes #9007 ## Test plan <img width="1110" alt="Rules table" src="https://github.com/astral-sh/ruff/assets/2586601/ac9fa682-623c-44aa-8e51-d8ab0d308355"> <img width="1110" alt="Rule page" src="https://github.com/astral-sh/ruff/assets/2586601/05850b2d-7ca5-49bb-8df8-bb931bab25cd">
Similar to #9689 — retains removed rules for better error messages and documentation but removed rules cannot be used in any context.
Removes PLR1706 as a useful test case and something we want to accomplish in #9680 anyway. The rule was in preview so we do not need to deprecate it first.
Closes #9007
Test plan