Update rule selection to respect preview mode #7195

zanieb · 2023-09-06T15:16:34Z

Summary

Extends work in #7046 (some relevant discussion there)

Changes:

All nursery rules are now referred to as preview rules
Documentation for the nursery is updated to describe preview
Adds a "PREVIEW" selector for preview rules
- This is primarily to allow --preview --ignore PREVIEW --extend-select FOO001,BAR200
Using --preview enables preview rules that match selectors

Notable decisions:

Preview rules are not selectable by their rule code without enabling preview
Retains the "NURSERY" selector for backwards compatibility
Nursery rules are selectable by their rule code for backwards compatiblity

Additional work:

Selection of preview rules without the "--preview" flag should display a warning
Use of deprecated nursery selection behavior should display a warning
Nursery selection should be removed after some time

Test Plan

Manual confirmation (i.e. we don't have an preview rules yet just nursery rules so I added a preview rule for manual testing)

New unit tests

zanieb · 2023-09-06T15:22:44Z

~~Note nursery selection backwards compatibility is still a work in progress.~~

github-actions · 2023-09-06T16:00:16Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

zanieb · 2023-09-06T16:14:29Z

crates/ruff_benchmark/benches/linter.rs

-        rules: RuleSelector::All.into_iter().collect(),
+        rules: RuleSelector::All.all_rules().collect(),


Presumably the benchmark regressions are due to including our nursery/preview rules now

Yep 9522dfb — I'll do this in a separate PR to avoid blocking this one

crates/ruff_dev/src/generate_docs.rs

crates/ruff/src/codes.rs

charliermarsh · 2023-09-07T14:18:09Z

crates/ruff/src/rule_selector.rs

+
+    // The rule must match the selector exactly.
+    rule.noqa_code().suffix() == prefix.short_code()
+}


I bet this could be encoded in the type system somehow, this feels awkward (I know that I wrote it lol). Not asking for a change here since it will likely involve more extensive changes in the macro, etc., just expressing that it feels strange.

I will open a tracking issue once this is merged

charliermarsh · 2023-09-07T14:21:08Z

ruff.schema.json

@@ -1883,17 +1885,22 @@
        "E1",
        "E10",
        "E101",
+        "E11",


There is some potential for confusion here that didn't exist in the nursery whereby if a user does --select E11 without --preview, we won't throw a hard error. Instead, we'll select no rules and emit no diagnostics (IIUC), though I imagine in the future we'll want to show a warning.

Yep #7210 will show a warning

charliermarsh

Just confirming (even though it's mentioned in the test plan) that no rules are actually moved to --preview here -- it's only enabling the functionality.

zanieb · 2023-09-07T14:51:56Z

@charliermarsh well all of the nursery rules will be enabled with --preview but yes there are no rules that are preview-only without being in the nursery.

crates/ruff/src/codes.rs

MichaReiser · 2023-09-07T15:14:15Z

crates/ruff/src/rule_selector.rs

            }
        }
    }
 }

+/// Returns `true` if the [`RuleCodePrefix`] matches a single rule exactly
+/// (e.g., `E225`, as opposed to `E2`).
+pub(crate) fn is_single_rule_selector(prefix: &RuleCodePrefix) -> bool {


Nit: Should this be a method on RuleCodePrefix?

Yeah maybe but it's in the macro 🤔

You can have multiple impl blocks even in different files. For as long as they are all in the same crate. Rust will merge them all together.

// macro generated impl RuleCodePrefix { } // somewhere else impl RuleCodePrefix { fn is_single_rule_selector(&self) -> bool { ... } }

MichaReiser · 2023-09-07T15:17:19Z

crates/ruff_cli/src/commands/rule.rs

@@ -19,7 +19,7 @@ struct Explanation<'a> {
    message_formats: &'a [&'a str],
    autofix: String,
    explanation: Option<&'a str>,
-    nursery: bool,
+    preview: bool,


Should we use PreviewMode here?

The rule is in preview and is enabled or disabled by preview mode :D I want to think a bit more about "preview" vs "preview mode" in general so thanks for pointing this out.

MichaReiser · 2023-09-07T15:19:06Z

crates/ruff_macros/src/map_codes.rs

            impl From<#linter> for crate::rule_selector::RuleSelector {
                fn from(linter: #linter) -> Self {
-                    Self::Prefix{prefix: RuleCodePrefix::#linter(linter), redirected_from: None}
+                    let prefix = RuleCodePrefix::#linter(linter);
+                    if is_single_rule_selector(&prefix) {
+                        Self::Rule {
+                            prefix,
+                            redirected_from: None,
+                        }
+                    } else {
+                        Self::Prefix {
+                            prefix,
+                            redirected_from: None,
+                        }
+                    }
                }


We could move all of this logic out of the macro if we instead implement impl From<#linter> for RuleCodePrefix because the RuleCodePrefix::#linter(linter) code is the only code that requires code-gen

Interesting. I'd like to separate further changes to the macro from this pull request though, if that's okay?

Yeah absolutely. I should have written this more clearly. This is feedback based on last week's discussion on how it probably would be possible to move a lot out of the macros if we abstracted the dynamic parts by traits instead of where we generate the implementations for our types. But this isn't something that concerns this PR. I just noticed it as a good example because the dynamic part is limited to a single line.

zanieb · 2023-09-07T23:29:38Z

I'd like to do a release tomorrow before merging this.

crates/ruff/src/rule_selector.rs

dhruvmanila · 2023-09-08T03:44:34Z

crates/ruff/src/rule_selector.rs

+    /// The specificity when selecting via a rule prefix at one-character depth (e.g., `--select PLE1`).
+    Prefix1Char,


I got a bit confused with "rule prefix" which made me think of the prefix part of the code i.e., "PLE" in the given example. But I think this is related to the number part, right? Maybe "Suffix1Char"?

Hm well the specificity is that it's a prefix e.g. including "PLE10" but I agree the character count part is confusing since it just refers to the code length.

…ules

…tor::Rule`

Temporarily. Looks to be causing benchmark regressions and should be considered separately.

Split out of #7195 so benchmark changes from enabling additional rules can be reviewed separately.

I don't love the sunrise emoji and 🧪 seems nice :) Requires #7195 --------- Co-authored-by: konsti <konstin@mailbox.org>

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [ruff](https://beta.ruff.rs/docs) ([source](https://togithub.com/astral-sh/ruff), [changelog](https://togithub.com/astral-sh/ruff/releases)) | `^0.0.288` -> `^0.0.289` | [![age](https://developer.mend.io/api/mc/badges/age/pypi/ruff/0.0.289?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/ruff/0.0.289?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/ruff/0.0.288/0.0.289?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/ruff/0.0.288/0.0.289?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>astral-sh/ruff (ruff)</summary> ### [`v0.0.289`](https://togithub.com/astral-sh/ruff/releases/tag/v0.0.289) [Compare Source](https://togithub.com/astral-sh/ruff/compare/v0.0.288...v0.0.289)  #### What's Changed ##### Bug Fixes - Invert condition for < and <= in outdated version block by [@charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7284](https://togithub.com/astral-sh/ruff/pull/7284) - Ignore `@override` method when enforcing `bad-dunder-name` rule by [@brendonh8](https://togithub.com/brendonh8) in [https://github.com/astral-sh/ruff/pull/7224](https://togithub.com/astral-sh/ruff/pull/7224) - Add `NotebookIndex` to the cache by [@dhruvmanila](https://togithub.com/dhruvmanila) in [https://github.com/astral-sh/ruff/pull/6863](https://togithub.com/astral-sh/ruff/pull/6863) ##### Preview This release includes a new preview mode which can be used to opt-in to unstable rules and features. - Update rule selection to respect preview mode by [@zanieb](https://togithub.com/zanieb) in [https://github.com/astral-sh/ruff/pull/7195](https://togithub.com/astral-sh/ruff/pull/7195) - Display the `--preview` option in the CLI help menu by [@zanieb](https://togithub.com/zanieb) in [https://github.com/astral-sh/ruff/pull/7274](https://togithub.com/astral-sh/ruff/pull/7274) See the [documentation](https://beta.ruff.rs/docs/preview/) and [versioning discussion](https://togithub.com/astral-sh/ruff/discussions/6998) for more details. #### New Contributors - [@brendonh8](https://togithub.com/brendonh8) made their first contribution in [https://github.com/astral-sh/ruff/pull/7224](https://togithub.com/astral-sh/ruff/pull/7224) **Full Changelog**: astral-sh/ruff@v0.0.288...v0.0.289 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/ixm-one/pytest-cmake-presets).  Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

This comment was marked as resolved.

Sign in to view

zanieb commented Sep 6, 2023

View reviewed changes

This was referenced Sep 6, 2023

Update preview and fix documentation symbols #7207

Merged

Enable preview mode during benchmarks #7208

Merged

Enable selection of preview rules #7046

Closed

zanieb force-pushed the zanie/rule-preview branch from 8490cf3 to 16f830f Compare September 6, 2023 21:10

zanieb marked this pull request as ready for review September 6, 2023 21:43

charliermarsh approved these changes Sep 7, 2023

View reviewed changes

charliermarsh reviewed Sep 7, 2023

View reviewed changes

MichaReiser reviewed Sep 7, 2023

View reviewed changes

dhruvmanila approved these changes Sep 8, 2023

View reviewed changes

zanieb and others added 14 commits September 8, 2023 16:58

Rename a bunch of "nursery" references to "preview"

9b4aee1

Update ALL rule selector to include all rules then deselect preview r…

8fce454

…ules

Clarify include_preview_rules conditional

c527f69

Fix conditional

a954fd6

WIP: Rename rule selector from nursery to preview

2e44d66

Enable selection of preview rules

51bce02

Make preview part of the selector API

1689190

Fix linter group comment

2c830e2

Add comment to PerFileIgnore

f111189

Restore nursery rule group for backwards compatible selection

e6d491a

Restore nursery selector

97b80c0

Fix macro generated RuleSelector::from(code) support for `RuleSelec…

2b0dfd6

…tor::Rule`

Fix pyproject test

359012a

Remove prefix_to_selector

b60ce8b

zanieb added 15 commits September 8, 2023 16:59

Add support for NURSERY selector (backwards compat)

83d888f

Disable preview mode during benchmarks

45ceb6a

Temporarily. Looks to be causing benchmark regressions and should be considered separately.

Separate test cases for selection

cb47f02

Add preview flag to test utility

2d8f961

Add tests for preview / nursery

550f787

Add test for select of linter; fix names of existing tests

c581006

Add test cases for selection of NURSERY and PREVIEW groups

f628e95

Fix typo

c092d62

Add unreachable code to test cases

32b34fa

Fix test case handling of RUF014

fbd449a

Fix preview mode messages

d94dfd7

Deprecate RuleSelector::Nursery

e7f095a

Deprecate RuleGroup::Nursery

b09fbb9

Fix deprecated usage in test

b374802

Improve prefix comments

4ea6f17

zanieb force-pushed the zanie/rule-preview branch from 8421200 to 4ea6f17 Compare September 8, 2023 21:59

zanieb mentioned this pull request Sep 10, 2023

JSON schema should contain "NURSERY" #7261

Closed

Fix unresolved merge conflict

2cab8b7

zanieb merged commit 6566d00 into main Sep 11, 2023
16 checks passed

zanieb deleted the zanie/rule-preview branch September 11, 2023 17:28

zanieb mentioned this pull request Sep 11, 2023

Docs build succeeds but is not published #7276

Closed

zanieb added the preview Related to preview mode features label Sep 11, 2023

zanieb added a commit that referenced this pull request Sep 11, 2023

Enable preview mode during benchmarks (#7208)

24b848a

Split out of #7195 so benchmark changes from enabling additional rules can be reviewed separately.

zanieb added a commit that referenced this pull request Sep 11, 2023

Update preview and fix documentation symbols (#7207)

73ad2af

I don't love the sunrise emoji and 🧪 seems nice :) Requires #7195 --------- Co-authored-by: konsti <konstin@mailbox.org>

chenrui333 mentioned this pull request Sep 12, 2023

ruff 0.0.289 Homebrew/homebrew-core#142113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update rule selection to respect preview mode #7195

Update rule selection to respect preview mode #7195

zanieb commented Sep 6, 2023 •

edited

Loading

zanieb commented Sep 6, 2023 •

edited

Loading

This comment was marked as resolved.

github-actions bot commented Sep 6, 2023 •

edited

Loading

zanieb Sep 6, 2023

zanieb Sep 6, 2023

zanieb Sep 6, 2023

charliermarsh Sep 7, 2023

zanieb Sep 7, 2023

charliermarsh Sep 7, 2023

zanieb Sep 7, 2023

charliermarsh left a comment

zanieb commented Sep 7, 2023

MichaReiser Sep 7, 2023

zanieb Sep 7, 2023

MichaReiser Sep 8, 2023

MichaReiser Sep 7, 2023

zanieb Sep 7, 2023

MichaReiser Sep 7, 2023

zanieb Sep 7, 2023

MichaReiser Sep 8, 2023 •

edited

Loading

zanieb commented Sep 7, 2023

dhruvmanila Sep 8, 2023

zanieb Sep 8, 2023

		rules: RuleSelector::All.into_iter().collect(),
		rules: RuleSelector::All.all_rules().collect(),

@@ @@ -1883,17 +1885,22 @@ @@
                       "E1",
                       "E10",
                       "E101",
+                      "E11",

		/// The specificity when selecting via a rule prefix at one-character depth (e.g., `--select PLE1`).
		Prefix1Char,

Update rule selection to respect preview mode #7195

Update rule selection to respect preview mode #7195

Conversation

zanieb commented Sep 6, 2023 • edited Loading

Summary

Test Plan

zanieb commented Sep 6, 2023 • edited Loading

This comment was marked as resolved.

github-actions bot commented Sep 6, 2023 • edited Loading

PR Check Results

Ecosystem

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh left a comment

Choose a reason for hiding this comment

zanieb commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

zanieb commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanieb commented Sep 6, 2023 •

edited

Loading

zanieb commented Sep 6, 2023 •

edited

Loading

github-actions bot commented Sep 6, 2023 •

edited

Loading

MichaReiser Sep 8, 2023 •

edited

Loading