Added ignoring deprecated rules for --select=ALL #10497
Open
+3
−6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WindowGenerator
changed the title
zanieb
reviewed
Mar 20, 2024
Comment on lines
189
to
194
| /// Return all matching rules, regardless of rule group filters like preview and deprecated. | ||
| pub fn all_rules(&self) -> impl Iterator<Item = Rule> + '_ { | ||
| match self { | ||
| RuleSelector::All => RuleSelectorIter::All(Rule::iter()), | ||
|
|
||
| RuleSelector::All => { | ||
| RuleSelectorIter::All(Rule::iter().filter(|rule| !rule.is_deprecated())) | ||
| } |
There was a problem hiding this comment.
Per the comment prefacing this function, we should not do this filtering here. Perhaps in rules() instead?
There was a problem hiding this comment.
Hey @WindowGenerator just want to check in again as we get closer to 0.4.0, did you want to work on this still?
There was a problem hiding this comment.
Hi @zanieb!
Yes, I will do it before the end of the weekend.
There was a problem hiding this comment.
Not a rush! Thanks for the reply.
There was a problem hiding this comment.
@zanieb Hi!
Code was changed, you can check it.
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| ANN101 | 34467 | 0 | 34467 | 0 | 0 |
| ANN102 | 652 | 0 | 652 | 0 | 0 |
| D107 | 2 | 1 | 1 | 0 | 0 |
Linter (preview)
✅ ecosystem check detected no linter changes.
|
Is this a breaking change because we remove rules from a selector? |
|
@MichaReiser that's a good call... maybe. It'd be easy to hold off merging until v0.4.0 which seems reasonable. |
|
It might be fine, at least from our versioning policy. The change is reasonable to me but it might be worth documenting that all selects all not deprecated rules to make it clear to users that they might "loose" rules even when upgrading to a new patch version |
WindowGenerator
force-pushed
the
deny-deprecated-rules-in-seleect-all-option
branch
2 times, most recently
from
fb70a9a
to
5577cb0
Compare
|
hi, this changed was planned for version what is the reason behind this choice? |
|
Hey @Alexdelia, we ended up releasing the parser in 0.4.0 and focused on delivery of that. We routinely move things to later milestones when they aren't high priority for the given release. |
zanieb
reviewed
Apr 24, 2024
Comment on lines
227
to
228
| // Deprecated rules are excluded in preview mode unless explicitly selected | ||
| || (rule.is_deprecated() && (!preview_enabled || self.is_exact())) | ||
| // Ignore deprecated rules for All option | ||
| || (rule.is_deprecated() && (!preview_enabled || self.is_exact()) && !matches!(self, RuleSelector::All { .. })) |
There was a problem hiding this comment.
Can you combine this comment with the one above? e.g.
Deprecated rules are excluded from ALL in stable require exact selection in preview
WindowGenerator
force-pushed
the
deny-deprecated-rules-in-seleect-all-option
branch
from
5577cb0
to
bb3f038
Compare
WindowGenerator
force-pushed
the
deny-deprecated-rules-in-seleect-all-option
branch
from
bb3f038
to
eeb9c3b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
#10342: Added ignoring deprecated rules for --select=ALL
Test Plan
Changed integration tests with --select=ALL option