From 27257e6340d77af511efa72107e159480da212f5 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 | 43 ++++++------ crates/ruff_cli/tests/format.rs | 73 ++++++++++++++++++--- crates/ruff_python_formatter/src/options.rs | 4 ++ crates/ruff_workspace/src/options.rs | 12 ++++ ruff.schema.json | 8 +-- 5 files changed, 104 insertions(+), 36 deletions(-) diff --git a/crates/ruff_cli/src/commands/format.rs b/crates/ruff_cli/src/commands/format.rs index d54c81828fa3e..54d135f296096 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,28 @@ 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.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.lines_between_types != isort_defaults.lines_between_types { - incompatible_options.push("'isort.lines_between_types'"); - } + // 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 setting.linter.isort.split_on_trailing_comma != isort_defaults.split_on_trailing_comma { - incompatible_options.push("'isort.split_on_trailing_comma'"); - } + // isort inserts a trailing comma which the formatter preserves, but only if `skip-magic-trailing-comma` isn't false. + if setting.formatter.magic_trailing_comma.is_ignore() { + if setting.linter.isort.force_wrap_aliases { + 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=false'."); + } - 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(", ")); + if setting.linter.isort.split_on_trailing_comma { + warn!("The isort option 'isort.split-on-trailing-comma' is incompatible with the formatter 'format.skip-magic-trailing-comma=true' option. To avoid unexpected behavior, we recommend either setting 'isort.split-on-trailing-comma=false' or 'format.skip-magic-trailing-comma=false'."); + } + } } } } diff --git a/crates/ruff_cli/tests/format.rs b/crates/ruff_cli/tests/format.rs index d0036d9ac9495..84974d9f0bf27 100644 --- a/crates/ruff_cli/tests/format.rs +++ b/crates/ruff_cli/tests/format.rs @@ -363,11 +363,14 @@ 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 +force-wrap-aliases = true +combine-as-imports = true split-on-trailing-comma = true + +[format] +skip-magic-trailing-comma = true "#, )?; @@ -390,7 +393,10 @@ 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=false'. + warning: The isort option 'isort.split-on-trailing-comma' is incompatible with the formatter 'format.skip-magic-trailing-comma=true' option. To avoid unexpected behavior, we recommend either setting 'isort.split-on-trailing-comma=false' or 'format.skip-magic-trailing-comma=false'. "###); Ok(()) } @@ -406,11 +412,14 @@ 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 +force-wrap-aliases = true +combine-as-imports = true split-on-trailing-comma = true + +[format] +skip-magic-trailing-comma = true "#, )?; @@ -429,7 +438,55 @@ 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=false'. + warning: The isort option 'isort.split-on-trailing-comma' is incompatible with the formatter 'format.skip-magic-trailing-comma=true' option. To avoid unexpected behavior, we recommend either setting 'isort.split-on-trailing-comma=false' or 'format.skip-magic-trailing-comma=false'. + "###); + 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 +split-on-trailing-comma = 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_python_formatter/src/options.rs b/crates/ruff_python_formatter/src/options.rs index c6eec75310ccf..b39c7b6ff79c6 100644 --- a/crates/ruff_python_formatter/src/options.rs +++ b/crates/ruff_python_formatter/src/options.rs @@ -250,6 +250,10 @@ impl MagicTrailingComma { pub const fn is_respect(self) -> bool { matches!(self, Self::Respect) } + + pub const fn is_ignore(self) -> bool { + matches!(self, Self::Ignore) + } } impl FromStr for MagicTrailingComma { diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 9a1cf8e60bc03..e895a444f2521 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. + /// + /// When using the formatter, ensure that `format.skip-magic-trailing-comma` is set to `false` (default) + /// when enabling `force-wrap-aliases` to avoid that the formatter collapses members if they all fit on a single line. #[option( default = r#"false"#, value_type = "bool", @@ -1726,6 +1729,9 @@ pub struct IsortOptions { /// the imports will never be folded into one line. /// /// See isort's [`split-on-trailing-comma`](https://pycqa.github.io/isort/docs/configuration/options.html#split-on-trailing-comma) option. + /// + /// When using the formatter, ensure that `format.skip-magic-trailing-comma` is set to `false` (default) when enabling `split-on-trailing-comma` + /// to avoid that the formatter removes the trailing commas. #[option( default = r#"true"#, value_type = "bool", @@ -1907,6 +1913,9 @@ pub struct IsortOptions { /// The number of blank lines to place after imports. /// Use `-1` for automatic determination. + /// + /// When using the formatter, only the values `-1`, `1`, and `2` are compatible because + /// it enforces at least one empty and at most two empty lines after imports. #[option( default = r#"-1"#, value_type = "int", @@ -1918,6 +1927,9 @@ pub struct IsortOptions { pub lines_after_imports: Option, /// The number of lines to place between "direct" and `import from` imports. + /// + /// When using the formatter, only the values `0` and `1` are compatible because + /// it 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 da6bdc761c201..117404e0c26cd 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\nWhen using the formatter, ensure that `format.skip-magic-trailing-comma` is set to `false` (default) when enabling `force-wrap-aliases` to avoid that the formatter collapses members if they all fit on a single 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\nWhen using the formatter, only the values `-1`, `1`, and `2` are compatible because it enforces at least one empty and at most two empty lines after imports.", "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\nWhen using the formatter, only the values `0` and `1` are compatible because it preserves up to one empty line after imports in nested blocks.", "type": [ "integer", "null" @@ -1559,7 +1559,7 @@ } }, "split-on-trailing-comma": { - "description": "If a comma is placed after the last member in a multi-line import, then the imports will never be folded into one line.\n\nSee isort's [`split-on-trailing-comma`](https://pycqa.github.io/isort/docs/configuration/options.html#split-on-trailing-comma) option.", + "description": "If a comma is placed after the last member in a multi-line import, then the imports will never be folded into one line.\n\nSee isort's [`split-on-trailing-comma`](https://pycqa.github.io/isort/docs/configuration/options.html#split-on-trailing-comma) option.\n\nWhen using the formatter, ensure that `format.skip-magic-trailing-comma` is set to `false` (default) when enabling `split-on-trailing-comma` to avoid that the formatter removes the trailing commas.", "type": [ "boolean", "null"