Add warnings for nursery and preview rule selection #7210

zanieb · 2023-09-06T21:08:40Z

Summary

Adds warnings for cases where:

A selector does not include any rules because preview is disabled
A nursery rule is selected without the preview flag

Test plan

Add integration tests

github-actions · 2023-09-06T21:59:41Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

zanieb · 2023-09-06T22:30:25Z

crates/ruff_workspace/src/configuration.rs

+        for selection in deprecated_nursery_selectors {
+            let (prefix, code) = selection.prefix_and_code();
+            warn_user!("Selection of nursery rule `{prefix}{code}` without the `--preview` flag is deprecated.",);
+        }


The redirect warning code has a comment that says this should be in the ruff_cli crate — how would we go about that? I'd like to create a tracking issue.

I'd vote to just remove that TODO -- I can't remember why it's there.

konstin · 2023-09-07T08:38:54Z

crates/ruff_cli/tests/integration_test.rs

+
+#[test]
+fn preview_enabled_prefix() {
+    // E741 and E225 (preview) should both be detected


What will happen to this test when E225 gets promoted to stable? (i don't have good answer for testing this)

Great question, do we choose a new code? Should we have a couple of rules in a "TEST" category that do nothing?

I'm looking into this

Not working... but an idea zanie/rule-warning...zanie/rule-tests

I'm a bit hesitant about adding test-only rules. We would need to filter them out in documentation, schema generation and prevent they can be selected in production (but then, we want them to be selectable in tests).

We could add the category behind a rust feature and make this an integration test (integration tests can require features), but this still suffers from the above-mentioned problems, although it has some more safeguards around it.

To me this is an inherent downside of our statically generated rule schema (instead of deriving a Rule array that has somewhat weaker keys which would allow you to mock out the rules).

I don't have a good solution to propose that doesn't require changing our rule structure or abstracting some of the logic by a trait that then allows overriding whether a rule is preview or not.

Doesn't the #[cfg(test)] approach address most of those concerns?

We can also wait and see how much of a burden it is to maintain the tests as-is. I don't like it though.

iirc #[cfg(test)] is only available in the same crate and not across crates (https://users.rust-lang.org/t/what-are-the-rules-for-cfg-test/54122/2), so e.g. ruff cfg(test) rules would not be available in ruff_cli.

Resources:

https://nrxus.github.io/faux/guide/exporting-mocks.html

make code compiled with cfg(test) visible across crates rust-lang/cargo#8379

If we're going to warn on use of NURSERY in #7210 we probably ought to show the `--preview` option in our help menus.

charliermarsh · 2023-09-12T19:42:06Z

crates/ruff_workspace/src/configuration.rs

+        for selection in deprecated_nursery_selectors {
+            let (prefix, code) = selection.prefix_and_code();
+            warn_user!("Selection of nursery rule `{prefix}{code}` without the `--preview` flag is deprecated.",);
+        }


I'd vote to just remove that TODO -- I can't remember why it's there.

charliermarsh · 2023-09-12T19:43:18Z

crates/ruff_workspace/src/configuration.rs

@@ -603,6 +628,18 @@ impl Configuration {
            );
        }

+        for selection in deprecated_nursery_selectors {
+            let (prefix, code) = selection.prefix_and_code();
+            warn_user!("Selection of nursery rule `{prefix}{code}` without the `--preview` flag is deprecated.",);


Because this doesn't use warn_user_once_by_id, the warnings will probably show up multiple times if we run this configuration resolution multiple times (which we might in a given invocation -- I can't remember exactly). Is it possible to use warn_user_once_by_id?

Yeah but I'd have to construct an id string like "nursery-{prefix}{code}", is that okay? I was thinking of adding a warn_user_once_by_message utility as an alternative? 🤷‍♀️

I think that's okay (to add an ID like that)

But I think the ID has to be a static string so that may not work...

charliermarsh · 2023-09-12T19:44:39Z

crates/ruff_workspace/src/configuration.rs

+                    // Check if the selector is empty because preview mode is disabled
+                    if selector.rules(PreviewMode::Disabled).next().is_none() {
+                        ignored_preview_selectors.insert(selector);
+                    }


Cool so this is like -- you selected CPY, but preview is disabled?

Yep! I think the exact code would warn too. We could make this "safer" by adding && selector.rules(PreviewMode::Enabled).next().is_some() but I think it's equivalent for now?

I can't write an integration test for exact codes yet since we don't have any preview rules.

charliermarsh · 2023-09-12T19:44:40Z

crates/ruff_workspace/src/configuration.rs

+                    // Check if the selector is empty because preview mode is disabled
+                    if selector.rules(PreviewMode::Disabled).next().is_none() {
+                        ignored_preview_selectors.insert(selector);
+                    }


And this is like -- you selected CPY, but preview is disabled?

Do both warnings show if you select CPY001? (Should they?)

Do you mean like https://github.com/astral-sh/ruff/pull/7210/files#diff-92738d4e248765a1e48e3c930a03e20ed306f9b0c23dd67ad72912133dbb10f2R289-R306? Or if you select CPY and CPY001?

I meant the former

charliermarsh · 2023-09-12T19:44:56Z

crates/ruff_workspace/src/configuration.rs

+                        if prefix.rules().any(|rule| rule.is_nursery()) {
+                            deprecated_nursery_selectors.insert(selector);
+                        }
+                    }


So this like: you selected CPY001, but preview is disabled.

crates/ruff_workspace/src/configuration.rs

charliermarsh

The warnings look reasonable to me.

One issue we may run into (which we've discussed before and I know you're aware of) is that users may want to enable a small number of preview rules, but don't want to enable (e.g.) the logical line rules, which will get enabled by default if they add --preview. Not sure how to resolve... Maybe being able to --ignore PREVIEW like you suggested. Anyway, not a block here, just coming to mind as I look at these deprecations.

codspeed-hq · 2023-09-12T20:11:51Z

CodSpeed Performance Report

Merging #7210 will degrade performances by 4.8%

_{Comparing zanie/rule-warning (0a067cc) with main (6566d00)}

Summary

❌ 5 regressions
✅ 20 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`zanie/rule-warning`	Change
❌	`linter/all-rules[large/dataset.py]`	154.5 ms	161.9 ms	-4.57%
❌	`linter/all-rules[numpy/globals.py]`	4 ms	4.1 ms	-2.6%
❌	`linter/all-rules[numpy/ctypeslib.py]`	33.2 ms	34.3 ms	-3.2%
❌	`linter/all-rules[unicode/pypinyin.py]`	15.4 ms	15.7 ms	-2.29%
❌	`linter/all-rules[pydantic/types.py]`	69.1 ms	72.6 ms	-4.8%

zanieb · 2023-09-12T20:34:06Z

Not sure how to resolve... Maybe being able to --ignore PREVIEW like you suggested. Anyway, not a block here, just coming to mind as I look at these deprecations.

Yeah I'm not sure what the proper solution for is either. We can hold off on merging the deprecations until we have a path forward? I don't have strong feelings, but we should find a decent migration story this week.

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

zanieb changed the title ~~zanie/rule warning~~ Add warnings for nursery and preview rule selection Sep 6, 2023

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

zanieb force-pushed the zanie/rule-warning branch 2 times, most recently from e1141e4 to 63b9c13 Compare September 6, 2023 21:42

zanieb commented Sep 6, 2023

View reviewed changes

konstin reviewed Sep 7, 2023

View reviewed changes

zanieb mentioned this pull request Sep 7, 2023

Update rule selection to respect preview mode #7195

Merged

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

Base automatically changed from zanie/rule-preview to main September 11, 2023 17:28

zanieb force-pushed the zanie/rule-warning branch from 07c52a7 to c9f58b8 Compare September 11, 2023 17:32

zanieb marked this pull request as ready for review September 11, 2023 17:32

zanieb mentioned this pull request Sep 11, 2023

Display the --preview option in the CLI help menu #7274

Merged

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

Display the --preview option in the CLI help menu (#7274)

c21b960

If we're going to warn on use of NURSERY in #7210 we probably ought to show the `--preview` option in our help menus.

charliermarsh reviewed Sep 12, 2023

View reviewed changes

crates/ruff_workspace/src/configuration.rs Outdated Show resolved Hide resolved

charliermarsh approved these changes Sep 12, 2023

View reviewed changes

zanieb added 6 commits September 13, 2023 15:00

Add warnings for nursery and preview rule selection

5ecb833

Cleanup

dfb760b

Add integration tests

0c69f82

Allow use of deprecated nursery in check

726206e

Format suggestion inline

ef961b7

Add test for direct rule selection of preview rule (not nursery)

167aeee

zanieb force-pushed the zanie/rule-warning branch from 0a067cc to 167aeee Compare September 13, 2023 20:04

zanieb merged commit ebd1b29 into main Sep 13, 2023

zanieb deleted the zanie/rule-warning branch September 13, 2023 20:30

zanieb mentioned this pull request Sep 13, 2023

Preview: Prevent duplicate warnings for repeated selections #7367

Open

zanieb mentioned this pull request Sep 13, 2023

Add internal rules for testing #7369

Closed

zanieb mentioned this pull request Jan 31, 2024

Add internal hidden rules for testing #9747

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warnings for nursery and preview rule selection #7210

Add warnings for nursery and preview rule selection #7210

zanieb commented Sep 6, 2023 •

edited

Loading

github-actions bot commented Sep 6, 2023 •

edited

Loading

zanieb Sep 6, 2023

charliermarsh Sep 12, 2023

konstin Sep 7, 2023

zanieb Sep 7, 2023

zanieb Sep 7, 2023

zanieb Sep 7, 2023

MichaReiser Sep 8, 2023

zanieb Sep 11, 2023

zanieb Sep 11, 2023

konstin Sep 12, 2023

zanieb Sep 12, 2023

charliermarsh Sep 12, 2023

charliermarsh Sep 12, 2023

zanieb Sep 12, 2023

charliermarsh Sep 12, 2023 •

edited

Loading

charliermarsh Sep 12, 2023

charliermarsh Sep 12, 2023

zanieb Sep 12, 2023

zanieb Sep 12, 2023

charliermarsh Sep 12, 2023

charliermarsh Sep 12, 2023

zanieb Sep 12, 2023

charliermarsh Sep 12, 2023

charliermarsh Sep 12, 2023

charliermarsh left a comment

codspeed-hq bot commented Sep 12, 2023

zanieb commented Sep 12, 2023

Add warnings for nursery and preview rule selection #7210

Add warnings for nursery and preview rule selection #7210

Conversation

zanieb commented Sep 6, 2023 • edited Loading

Summary

Test plan

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

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 Sep 12, 2023 • edited Loading

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

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

codspeed-hq bot commented Sep 12, 2023

Merging #7210 will degrade performances by 4.8%

Summary

Benchmarks breakdown

zanieb commented Sep 12, 2023

zanieb commented Sep 6, 2023 •

edited

Loading

github-actions bot commented Sep 6, 2023 •

edited

Loading

charliermarsh Sep 12, 2023 •

edited

Loading