From e0f34f9b1b6e4b10a90fd12532abae0cdcf611dc Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Wed, 25 Oct 2023 10:30:01 +0900 Subject: [PATCH] Refine warnings about incompatible `isort` options --- crates/ruff_cli/src/commands/format.rs | 41 ++++++--------- crates/ruff_cli/tests/format.rs | 72 ++++++++++++++++++++++---- crates/ruff_workspace/src/options.rs | 9 ++++ ruff.schema.json | 6 +-- 4 files changed, 90 insertions(+), 38 deletions(-) diff --git a/crates/ruff_cli/src/commands/format.rs b/crates/ruff_cli/src/commands/format.rs index d54c81828fa3e..580b00c84222a 100644 --- a/crates/ruff_cli/src/commands/format.rs +++ b/crates/ruff_cli/src/commands/format.rs @@ -19,7 +19,6 @@ use ruff_diagnostics::SourceMap; use ruff_linter::fs; use ruff_linter::logging::LogLevel; use ruff_linter::registry::Rule; -use ruff_linter::rules::isort; use ruff_linter::settings::rule_table::RuleTable; use ruff_linter::source_kind::{SourceError, SourceKind}; use ruff_linter::warn_user_once; @@ -713,32 +712,24 @@ pub(super) fn warn_incompatible_formatter_settings( warn!("The following rules may cause conflicts when used with the formatter: {}. To avoid unexpected behavior, we recommend disabling these rules, either by removing them from the `select` or `extend-select` configuration, or adding then to the `ignore` configuration.", incompatible_rules.join(", ")); } - let mut incompatible_options = Vec::new(); - - let isort_defaults = isort::settings::Settings::default(); - - if setting.linter.isort.force_single_line != isort_defaults.force_single_line { - incompatible_options.push("'isort.force-single-line'"); - } - - if setting.linter.isort.force_wrap_aliases != isort_defaults.force_wrap_aliases { - incompatible_options.push("'isort.force-wrap-aliases'"); - } - - if setting.linter.isort.lines_after_imports != isort_defaults.lines_after_imports { - incompatible_options.push("'isort.lines-after-imports'"); - } - - if setting.linter.isort.lines_between_types != isort_defaults.lines_between_types { - incompatible_options.push("'isort.lines_between_types'"); - } + if setting.linter.rules.enabled(Rule::UnsortedImports) { + // The formatter removes empty lines if the value is larger than 2 but always inserts a empty line after imports. + // Two empty lines are okay because `isort` only uses this setting for top-level imports (not in nested blocks). + if !matches!(setting.linter.isort.lines_after_imports, 1 | 2 | -1) { + warn!("The isort option 'isort.lines-after-imports' with a value other than `-1`, `1` or `2` is incompatible with the formatter. To avoid unexpected behavior, we recommend setting the option to one of: `2`, `1`, or `-1` (default)."); + } - if setting.linter.isort.split_on_trailing_comma != isort_defaults.split_on_trailing_comma { - incompatible_options.push("'isort.split_on_trailing_comma'"); - } + // Values larger than two get reduced to one line by the formatter if the import is in a nested block. + if setting.linter.isort.lines_between_types > 1 { + warn!("The isort option 'isort.lines-between-types' with a value larger than 1 is incompatible with the formatter. To avoid unexpected behavior, we recommend setting the option to one of: `1` or `0` (default)."); + } - if !incompatible_options.is_empty() { - warn!("The following isort options may cause conflicts when used with the formatter: {}. To avoid unexpected behavior, we recommend disabling these options by removing them from the configuration.", incompatible_options.join(", ")); + // Isort always inserts a trailing comma which the formatter preserves, but only if `skip-magic-trailing-comma` isn't false. + if setting.linter.isort.force_wrap_aliases + && !setting.formatter.magic_trailing_comma.is_respect() + { + warn!("The isort option 'isort.force-wrap-aliases' is incompatible with the formatter 'format.skip-magic-trailing-comma=true' option. To avoid unexpected behavior, we recommend either setting 'isort.force-wrap-aliases=false' or 'format.skip-magic-trailing-comma=true'."); + } } } } diff --git a/crates/ruff_cli/tests/format.rs b/crates/ruff_cli/tests/format.rs index d0036d9ac9495..5eb5fad3c3636 100644 --- a/crates/ruff_cli/tests/format.rs +++ b/crates/ruff_cli/tests/format.rs @@ -363,11 +363,13 @@ select = ["ALL"] ignore = ["D203", "D212"] [isort] -force-single-line = true -force-wrap-aliases = true -lines-after-imports = 0 +lines-after-imports = 3 lines-between-types = 2 -split-on-trailing-comma = true +force-wrap-aliases = true +combine-as-imports = true + +[format] +skip-magic-trailing-comma = true "#, )?; @@ -390,7 +392,9 @@ def say_hy(name: str): ----- stderr ----- warning: The following rules may cause conflicts when used with the formatter: 'COM812', 'COM819', 'D206', 'E501', 'ISC001', 'Q000', 'Q001', 'Q002', 'Q003', 'W191'. To avoid unexpected behavior, we recommend disabling these rules, either by removing them from the `select` or `extend-select` configuration, or adding then to the `ignore` configuration. - warning: The following isort options may cause conflicts when used with the formatter: 'isort.force-single-line', 'isort.force-wrap-aliases', 'isort.lines-after-imports', 'isort.lines_between_types'. To avoid unexpected behavior, we recommend disabling these options by removing them from the configuration. + warning: The isort option 'isort.lines-after-imports' with a value other than `-1`, `1` or `2` is incompatible with the formatter. To avoid unexpected behavior, we recommend setting the option to one of: `2`, `1`, or `-1` (default). + warning: The isort option 'isort.lines-between-types' with a value larger than 1 is incompatible with the formatter. To avoid unexpected behavior, we recommend setting the option to one of: `1` or `0` (default). + warning: The isort option 'isort.force-wrap-aliases' is incompatible with the formatter 'format.skip-magic-trailing-comma=true' option. To avoid unexpected behavior, we recommend either setting 'isort.force-wrap-aliases=false' or 'format.skip-magic-trailing-comma=true'. "###); Ok(()) } @@ -406,11 +410,13 @@ select = ["ALL"] ignore = ["D203", "D212"] [isort] -force-single-line = true -force-wrap-aliases = true -lines-after-imports = 0 +lines-after-imports = 3 lines-between-types = 2 -split-on-trailing-comma = true +force-wrap-aliases = true +combine-as-imports = true + +[format] +skip-magic-trailing-comma = true "#, )?; @@ -429,7 +435,53 @@ def say_hy(name: str): ----- stderr ----- warning: The following rules may cause conflicts when used with the formatter: 'COM812', 'COM819', 'D206', 'E501', 'ISC001', 'Q000', 'Q001', 'Q002', 'Q003', 'W191'. To avoid unexpected behavior, we recommend disabling these rules, either by removing them from the `select` or `extend-select` configuration, or adding then to the `ignore` configuration. - warning: The following isort options may cause conflicts when used with the formatter: 'isort.force-single-line', 'isort.force-wrap-aliases', 'isort.lines-after-imports', 'isort.lines_between_types'. To avoid unexpected behavior, we recommend disabling these options by removing them from the configuration. + warning: The isort option 'isort.lines-after-imports' with a value other than `-1`, `1` or `2` is incompatible with the formatter. To avoid unexpected behavior, we recommend setting the option to one of: `2`, `1`, or `-1` (default). + warning: The isort option 'isort.lines-between-types' with a value larger than 1 is incompatible with the formatter. To avoid unexpected behavior, we recommend setting the option to one of: `1` or `0` (default). + warning: The isort option 'isort.force-wrap-aliases' is incompatible with the formatter 'format.skip-magic-trailing-comma=true' option. To avoid unexpected behavior, we recommend either setting 'isort.force-wrap-aliases=false' or 'format.skip-magic-trailing-comma=true'. + "###); + Ok(()) +} + +#[test] +fn valid_linter_options() -> Result<()> { + let tempdir = TempDir::new()?; + let ruff_toml = tempdir.path().join("ruff.toml"); + fs::write( + &ruff_toml, + r#" +select = ["ALL"] +ignore = ["D203", "D212"] + +[isort] +lines-after-imports = 2 +lines-between-types = 1 +force-wrap-aliases = true +combine-as-imports = true + +[format] +skip-magic-trailing-comma = false +"#, + )?; + + let test_path = tempdir.path().join("test.py"); + fs::write( + &test_path, + r#" +def say_hy(name: str): + print(f"Hy {name}")"#, + )?; + + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(["format", "--no-cache", "--config"]) + .arg(&ruff_toml) + .arg(test_path), @r###" + success: true + exit_code: 0 + ----- stdout ----- + 1 file reformatted + + ----- stderr ----- + warning: The following rules may cause conflicts when used with the formatter: 'COM812', 'COM819', 'D206', 'E501', 'ISC001', 'Q000', 'Q001', 'Q002', 'Q003', 'W191'. To avoid unexpected behavior, we recommend disabling these rules, either by removing them from the `select` or `extend-select` configuration, or adding then to the `ignore` configuration. "###); Ok(()) } diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index ace00a2463b27..471d59f9c2328 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -1683,6 +1683,9 @@ pub struct IsortOptions { /// `combine-as-imports = true`. When `combine-as-imports` isn't /// enabled, every aliased `import from` will be given its own line, in /// which case, wrapping is not necessary. + /// + /// Using this setting with the formatter requires that `format.skip-magic-trailing-comma` is set to `false` (default) + /// to avoid that the formatter collapses members if they all fit on the line. #[option( default = r#"false"#, value_type = "bool", @@ -1907,6 +1910,9 @@ pub struct IsortOptions { /// The number of blank lines to place after imports. /// Use `-1` for automatic determination. + /// + /// Note: Value's other than `-1`, `1`, or `2` are incompatible with the formatter because it + /// enforces at least one emty and at most two empty lines after import . #[option( default = r#"-1"#, value_type = "int", @@ -1918,6 +1924,9 @@ pub struct IsortOptions { pub lines_after_imports: Option, /// The number of lines to place between "direct" and `import from` imports. + /// + /// Note: Values larger than 1 are incompatible with the formatter because it only + /// preserves up to one empty line after imports in nested blocks. #[option( default = r#"0"#, value_type = "int", diff --git a/ruff.schema.json b/ruff.schema.json index fb57ef8d84801..21b8309a39fd6 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1424,7 +1424,7 @@ } }, "force-wrap-aliases": { - "description": "Force `import from` statements with multiple members and at least one alias (e.g., `import A as B`) to wrap such that every line contains exactly one member. For example, this formatting would be retained, rather than condensing to a single line:\n\n```python from .utils import ( test_directory as test_directory, test_id as test_id ) ```\n\nNote that this setting is only effective when combined with `combine-as-imports = true`. When `combine-as-imports` isn't enabled, every aliased `import from` will be given its own line, in which case, wrapping is not necessary.", + "description": "Force `import from` statements with multiple members and at least one alias (e.g., `import A as B`) to wrap such that every line contains exactly one member. For example, this formatting would be retained, rather than condensing to a single line:\n\n```python from .utils import ( test_directory as test_directory, test_id as test_id ) ```\n\nNote that this setting is only effective when combined with `combine-as-imports = true`. When `combine-as-imports` isn't enabled, every aliased `import from` will be given its own line, in which case, wrapping is not necessary.\n\nUsing this setting with the formatter requires that `format.skip-magic-trailing-comma` is set to `false` (default) to avoid that the formatter collapses members if they all fit on the line.", "type": [ "boolean", "null" @@ -1471,7 +1471,7 @@ } }, "lines-after-imports": { - "description": "The number of blank lines to place after imports. Use `-1` for automatic determination.", + "description": "The number of blank lines to place after imports. Use `-1` for automatic determination.\n\nNote: Value's other than `-1`, `1`, or `2` are incompatible with the formatter because it enforces at least one emty and at most two empty lines after import .", "type": [ "integer", "null" @@ -1479,7 +1479,7 @@ "format": "int" }, "lines-between-types": { - "description": "The number of lines to place between \"direct\" and `import from` imports.", + "description": "The number of lines to place between \"direct\" and `import from` imports.\n\nNote: Values larger than 1 are incompatible with the formatter because it only preserves up to one empty line after imports in nested blocks.", "type": [ "integer", "null"