-
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 deprecation infrastructure #9689
Conversation
|
Question: Is there a planned mechanism to explain why a particularly rule was deprecated? It might be helpful for a brief explanation to be available in the docs or a verbose error message. |
Yeah, we should probably add a section to the documentation for that particular rule. |
Also, it would be nice to mention planned version (if any) of its drop time: |
Yeah this would be nice. I'm not sure how to do this with our macro but I should try to figure it out.
I don't think we can feasibly include "will be dropped in" versions — they're too likely to go out of date since builds are static. It sounds nice, but hard to maintain. |
I've added deprecation status messages to:
I've also added specific explanations for the deprecations to each rule's documentation. |
table_out.push_str(&format!( | ||
"{SPACER}{FIX_SYMBOL}{SPACER} The rule is automatically fixable by the `--fix` command-line option." | ||
)); | ||
table_out.push_str("<br />"); |
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 kind of preferred what we had before over a separate section, but I assume you tried to just extend the previous layout and this looked better?
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.
For the legend specifically? I didn't like how much blank space there was so I tried removing that and it felt awkward. I find having all of sentences weirdly verbose for documenting the meanings of the icons.
One thing we could do here is link to the legend from the icons in the rule table. I've tried clicking them multiple times :D
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 interesting. We could even put it at the bottom I guess.
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.
Let's focus on this separately #9711
(Flake8Annotations, "101") => (RuleGroup::Stable, rules::flake8_annotations::rules::MissingTypeSelf), | ||
(Flake8Annotations, "102") => (RuleGroup::Stable, rules::flake8_annotations::rules::MissingTypeCls), | ||
(Flake8Annotations, "101") => (RuleGroup::Deprecated, rules::flake8_annotations::rules::MissingTypeSelf), | ||
(Flake8Annotations, "102") => (RuleGroup::Deprecated, rules::flake8_annotations::rules::MissingTypeCls), |
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'm less certain on ANN102
. I would never enable it myself, but could it have value?
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 can't see why. It think the annotations here are either self: Self
or cls: type[Self]
. Are there situations in which they would differ for the cls
case? cc @AlexWaygood
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.
PEP-673 has a really annoying restriction that says you can't use typing.Self
in metaclasses, so you have to use a custom TypeVar for __new__
methods in metaclasses (in typeshed we work around this in a somewhat-hacky way: we have a custom TypeVar that is literally just defined as Self = TypeVar("Self")
, and use that for these situations: https://github.com/python/typeshed/blob/3881b1b81a8118aa6fc2d2e9616c8f541e5101ed/stdlib/builtins.pyi#L194-L196)
You also might need to use a custom type variable for cls
if you can't use typing.Self
for whatever reason -- maybe you support older Pythons and you can't declare a dependency on typing_extensions
?
But I agree that this seems like a pretty pointless rule, honestly. The vast majority of the time, you can leave these unannotated, and no type checker is going to care if you leave them unannotated. If you need to annotate cls
, you'll know that you need to do so without a lint rule telling you about it, I think
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">
Adds a new `Deprecated` rule group in addition to `Stable` and `Preview`. Deprecated rules: - Warn on explicit selection without preview - Error on explicit selection with preview - Are excluded when selected by prefix with preview Deprecates `TRY200`, `ANN101`, and `ANN102` as a proof of concept. We can consider deprecating them separately.
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
Adds a new `Deprecated` rule group in addition to `Stable` and `Preview`. Deprecated rules: - Warn on explicit selection without preview - Error on explicit selection with preview - Are excluded when selected by prefix with preview Deprecates `TRY200`, `ANN101`, and `ANN102` as a proof of concept. We can consider deprecating them separately.
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">
Adds a new `Deprecated` rule group in addition to `Stable` and `Preview`. Deprecated rules: - Warn on explicit selection without preview - Error on explicit selection with preview - Are excluded when selected by prefix with preview Deprecates `TRY200`, `ANN101`, and `ANN102` as a proof of concept. We can consider deprecating them separately.
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">
Adds a new
Deprecated
rule group in addition toStable
andPreview
.Deprecated rules:
Deprecates
TRY200
,ANN101
, andANN102
as a proof of concept. We can consider deprecating them separately.