[`flake8-import-conventions`] Syntax check aliases supplied in configuration for `unconventional-import-alias (ICN001)` #14477

dylwil3 · 2024-11-20T05:01:51Z

This PR introduces a validation step to the deserialization of the configuration for unconventional-import-alias (ICN001). We verify that the import module and alias supplied have valid syntax in order to avoid a panic if these aliases are added in during a fix.

Closes #14439

dylwil3 · 2024-11-20T05:04:06Z

Two notes:

Sorry for bloating the workspace crate with serde and especially with serde_json (since I only use the latter for a test)... let me know if you'd like me to work around that
I don't add a deserialization check for "banned aliases" or "banned from" since a fix would never add those to the source code

github-actions · 2024-11-20T05:11:42Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

MichaReiser · 2024-11-20T07:33:34Z

crates/ruff_workspace/Cargo.toml

@@ -42,6 +43,7 @@ serde = { workspace = true }
 shellexpand = { workspace = true }
 strum = { workspace = true }
 toml = { workspace = true }
+serde_json.workspace = true


This should be a dev-dependency if it is only used in tests

No longer used (test moved to integration test)

MichaReiser · 2024-11-20T07:35:28Z

crates/ruff_workspace/src/options.rs

+    #[test]
+    fn flake8_import_conventions_validate_aliases() {
+        let json_options = r#"{"aliases": {"a.b":"a.b"}}"#;
+        let result: Result<Flake8ImportConventionsOptions, _> = serde_json::from_str(json_options);
+        assert!(result.unwrap_err().to_string().contains("Module must be valid identifier separated by single periods and alias must be valid identifier"));
+    }
+


I'm leaning towards making this a ruff cli integration test. It then also demonstrates how the error message is displayed to the users.

See

ruff/crates/ruff/tests/lint.rs

Line 23 in c847cad

fn top_level_options() -> Result<()> {

MichaReiser · 2024-11-20T07:36:14Z

crates/ruff_workspace/src/options.rs

+        if module.is_empty()
+            || module.split('.').any(|part| !is_identifier(part))
+            || !is_identifier(alias)
+        {
+            return Err(de::Error::custom(
+                "Module must be valid identifier separated by single periods and alias must be valid identifier",
+            ));
+        }


I suggest that we use two dedicated errors: One for alias and one for module.

AlexWaygood · 2024-11-20T08:48:21Z

Let's try to get this into ruff 0.8 if we can — while it is a bugfix, it might still be breaking for some users who are currently "using the rule incorrectly"

AlexWaygood · 2024-11-20T08:52:26Z

Sorry — I optimistically tried to change the target branch, but it messed up the diff, so I swiftly reverted that back 😅

It's probably fine if this goes straight into main, though, as long as we merge it before the 0.8 release goes out. I highly doubt we'll be cutting another patch release before 0.8, since everything is roughly on schedule for the minor release

MichaReiser · 2024-11-20T12:08:37Z

I also don't consider this breaking. It doesn't change the intent of the rule.

AlexWaygood · 2024-11-20T12:12:02Z

I also don't consider this breaking. It doesn't change the intent of the rule.

I agree that it doesn't change the intent of the rule, but clearly some users are not using the rule as it was intended: #14439 (comment). We will break those users; this is a case of Hyrum's law.

I strongly agree with the change, I'd just prefer to have it go into Ruff 0.8 and to list it in BREAKING_CHANGES.md, out of an abundance of caution.

dylwil3 · 2024-11-20T19:20:16Z

I agree that it doesn't change the intent of the rule, but clearly some users are not using the rule as it was intended: #14439 (comment). We will break those users; this is a case of Hyrum's law.

Relevant xkcd

MichaReiser

Thanks for adding the integration tests. They show that the error messages aren't great. We should either:

minimal: Include the alias and module name in the error message
better: Use newtype wrappers for Module and Alias (FxHashMap<ModuleName, Alias>) and implement Deserialize on the newtype wrappers. That should allow serde to highlight the relevant line and column in the error message.

MichaReiser · 2024-11-20T20:18:29Z

crates/ruff/tests/lint.rs

+  Cause: Failed to parse [TMP]/ruff.toml
+  Cause: TOML parse error at line 2, column 2
+  |
+2 | [lint.flake8-import-conventions.aliases]


Hmm. The errors aren't great. Let's at least include the alias in the error message to help users figure out what's going on. The same for the module name.

MichaReiser · 2024-11-20T20:20:26Z

crates/ruff_workspace/src/options.rs

@@ -1327,6 +1329,7 @@ pub struct Flake8ImportConventionsOptions {
            scipy = "sp"
        "#
    )]
+    #[serde(deserialize_with = "deserialize_and_validate_aliases")]
    pub aliases: Option<FxHashMap<String, String>>,


We would probably get better error messages if you create a newtype for ModuleName and Alias and implement Deserialize for them. I would expect that serde can then narrow the error message to the specific line rather than the entire table

This PR does actually already seem to be a small improvement on the status quo when it comes to line numbers. Currently if I make this change to typeshed's pyproject.toml file:

diff --git a/pyproject.toml b/pyproject.toml index 46014bcd2..927da6162 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -111,6 +111,9 @@ ignore = [ "E501", # Line too long ] +[tool.ruff.lint.flake8-import-conventions.aliases] +"pandas" = 42 +

Then Ruff v0.7.1 gives me this error message:

ruff failed Cause: Failed to parse /Users/alexw/dev/typeshed/pyproject.toml Cause: TOML parse error at line 27, column 1 | 27 | [tool.ruff.lint] | ^^^^^^^^^^^^^^^^ invalid type: integer `42`, expected a string

MichaReiser · 2024-11-21T15:40:35Z

I pushed a change that implements Deserialize for the key and value. I'm surprised that serde isn't better at handling this.

MichaReiser

@AlexWaygood should we merge?

AlexWaygood

Thanks @dylwil3!

dylwil3 · 2024-11-21T15:51:43Z

@MichaReiser you beat me to it! Much improved error message, thanks!

dylwil3 added 2 commits November 19, 2024 22:56

add serde dependencies to ruff_workspace

5a3b3ce

custom deserialization for aliases

83b5b5f

MichaReiser added the configuration Related to settings and configuration label Nov 20, 2024

MichaReiser reviewed Nov 20, 2024

View reviewed changes

MichaReiser added bug Something isn't working and removed configuration Related to settings and configuration labels Nov 20, 2024

MichaReiser reviewed Nov 20, 2024

View reviewed changes

AlexWaygood added this to the v0.8 milestone Nov 20, 2024

AlexWaygood changed the base branch from main to ruff-0.8 November 20, 2024 08:49

AlexWaygood requested review from carljm, AlexWaygood and sharkdp as code owners November 20, 2024 08:49

AlexWaygood changed the base branch from ruff-0.8 to main November 20, 2024 08:49

AlexWaygood removed request for carljm, sharkdp and AlexWaygood November 20, 2024 08:49

AlexWaygood added the breaking Breaking API change label Nov 20, 2024

dylwil3 added 3 commits November 20, 2024 13:16

remove test and correct error msg capitalization as per serde suggestion

da079b7

remove unused dependency

bb3f6aa

add integration test

6a3ad60

clippy

674bf6f

MichaReiser requested changes Nov 20, 2024

View reviewed changes

AlexWaygood removed this from the v0.8 milestone Nov 21, 2024

Improve error messages

fbcfdad

MichaReiser requested a review from AlexWaygood November 21, 2024 15:40

MichaReiser approved these changes Nov 21, 2024

View reviewed changes

error message nits

14ad456

MichaReiser force-pushed the unconventional-alias branch from 14ad456 to 4e0cf71 Compare November 21, 2024 15:46

schema

e1805e9

AlexWaygood force-pushed the unconventional-alias branch from 4e0cf71 to e1805e9 Compare November 21, 2024 15:48

AlexWaygood approved these changes Nov 21, 2024

View reviewed changes

MichaReiser added configuration Related to settings and configuration and removed bug Something isn't working labels Nov 21, 2024

MichaReiser enabled auto-merge (squash) November 21, 2024 15:51

MichaReiser removed the breaking Breaking API change label Nov 21, 2024

MichaReiser merged commit 2efa3fb into astral-sh:main Nov 21, 2024
19 checks passed

AlexWaygood added this to the v0.8 milestone Nov 21, 2024

MichaReiser added a commit that referenced this pull request Nov 21, 2024

Add #14477

89b6437

BrewTestBot mentioned this pull request Nov 22, 2024

ruff 0.8.0 Homebrew/homebrew-core#198630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`flake8-import-conventions`] Syntax check aliases supplied in configuration for `unconventional-import-alias (ICN001)` #14477

[`flake8-import-conventions`] Syntax check aliases supplied in configuration for `unconventional-import-alias (ICN001)` #14477

dylwil3 commented Nov 20, 2024

dylwil3 commented Nov 20, 2024

github-actions bot commented Nov 20, 2024 •

edited

Loading

MichaReiser Nov 20, 2024

dylwil3 Nov 20, 2024

MichaReiser Nov 20, 2024

dylwil3 Nov 20, 2024

MichaReiser Nov 20, 2024

dylwil3 Nov 20, 2024

AlexWaygood commented Nov 20, 2024

AlexWaygood commented Nov 20, 2024 •

edited

Loading

MichaReiser commented Nov 20, 2024

AlexWaygood commented Nov 20, 2024

dylwil3 commented Nov 20, 2024

MichaReiser left a comment

MichaReiser Nov 20, 2024

MichaReiser Nov 20, 2024

AlexWaygood Nov 21, 2024 •

edited

Loading

MichaReiser commented Nov 21, 2024

MichaReiser left a comment

AlexWaygood left a comment

dylwil3 commented Nov 21, 2024

[flake8-import-conventions] Syntax check aliases supplied in configuration for unconventional-import-alias (ICN001) #14477

[flake8-import-conventions] Syntax check aliases supplied in configuration for unconventional-import-alias (ICN001) #14477

Conversation

dylwil3 commented Nov 20, 2024

dylwil3 commented Nov 20, 2024

github-actions bot commented Nov 20, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

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

AlexWaygood commented Nov 20, 2024

AlexWaygood commented Nov 20, 2024 • edited Loading

MichaReiser commented Nov 20, 2024

AlexWaygood commented Nov 20, 2024

dylwil3 commented Nov 20, 2024

MichaReiser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWaygood Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

MichaReiser commented Nov 21, 2024

MichaReiser left a comment

Choose a reason for hiding this comment

AlexWaygood left a comment

Choose a reason for hiding this comment

dylwil3 commented Nov 21, 2024

[`flake8-import-conventions`] Syntax check aliases supplied in configuration for `unconventional-import-alias (ICN001)` #14477

[`flake8-import-conventions`] Syntax check aliases supplied in configuration for `unconventional-import-alias (ICN001)` #14477

github-actions bot commented Nov 20, 2024 •

edited

Loading

`ruff-ecosystem` results

AlexWaygood commented Nov 20, 2024 •

edited

Loading

AlexWaygood Nov 21, 2024 •

edited

Loading