Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn about incompatible formatter options #8088

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Warn about incompatible formatter options #8088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 75 additions & 2 deletions crates/ruff_cli/src/commands/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::time::Instant;
use anyhow::Result;
use colored::Colorize;
use itertools::Itertools;
use log::error;
use log::{error, warn};
use rayon::iter::Either::{Left, Right};
use rayon::iter::{IntoParallelRefIterator, ParallelIterator};
use similar::TextDiff;
Expand All @@ -18,12 +18,17 @@ use tracing::debug;
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;
use ruff_python_ast::{PySourceType, SourceType};
use ruff_python_formatter::{format_module_source, FormatModuleError};
use ruff_text_size::{TextLen, TextRange, TextSize};
use ruff_workspace::resolver::{match_exclusion, python_files_in_path, ResolvedFile};
use ruff_workspace::resolver::{
match_exclusion, python_files_in_path, PyprojectConfig, ResolvedFile, Resolver,
};
use ruff_workspace::FormatterSettings;

use crate::args::{CliOverrides, FormatArguments};
Expand Down Expand Up @@ -74,6 +79,8 @@ pub(crate) fn format(
return Ok(ExitStatus::Success);
}

warn_incompatible_formatter_settings(&pyproject_config, Some(&resolver));

// Discover the package root for each Python file.
let package_roots = resolver.package_roots(
&paths
Expand Down Expand Up @@ -638,3 +645,69 @@ impl Display for FormatCommandError {
}
}
}

pub(super) fn warn_incompatible_formatter_settings(
pyproject_config: &PyprojectConfig,
resolver: Option<&Resolver>,
) {
for setting in std::iter::once(&pyproject_config.settings)
.chain(resolver.iter().flat_map(|resolver| resolver.settings()))
{
let mut incompatible_rules = Vec::new();

for incompatible_rule in RuleTable::from_iter([
Rule::LineTooLong,
Rule::TabIndentation,
Rule::IndentationWithInvalidMultiple,
Rule::IndentationWithInvalidMultipleComment,
Rule::OverIndented,
Rule::IndentWithSpaces,
Rule::SingleLineImplicitStringConcatenation,
Rule::MissingTrailingComma,
Rule::ProhibitedTrailingComma,
Rule::BadQuotesInlineString,
Rule::BadQuotesMultilineString,
Rule::BadQuotesDocstring,
Rule::AvoidableEscapedQuote,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tricky thing here is that these don't necessarily conflict with the formatter. Some of them do overlap, but in a way that's at least consistent. Some of them are also arguably complimentary (like MissingTrailingComma, I think, is arguably complimentary, since it will insert trailing commas that the formatter will then respect).

Copy link
Member Author

@MichaReiser MichaReiser Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COM812 is conflicting because it disallows:

class PhotoMetadata:
    """Metadata about a photo."""

    def __init__(
        self, file: Path, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t, u
    ):
        ...

My goal is to only warn about rules that are incompatible. E.g. I excluded

  • E501 because there are valid use cases to use it alongside the formatter, even though it's mostly redundant
  • E701: while redundant, it doesn't conflict with the formatter.

Is your feedback mainly about the naming of the variables or the wording of the warning message? I'm trying to understand what I should change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm just trying to understand what we consider to be a conflict (and, based on that answer, perhaps looking to refine the list, or change the wording of the message).

For example, above, if you ran the linter, wouldn't COM812 just insert a trailing comma at the end, and then the formatter would in turn split the arguments over multiple lines? So it's not like the two tools would go back and forth endlessly disagreeing with one another, as they would if you set BadQuotes to divergent values. The COM812 case feels like it fits some definition of a conflict though, like: formatted code can introduce these violations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, above, if you ran the linter, wouldn't COM812 just insert a trailing comma at the end, and then the formatter would in turn split the arguments over multiple lines?

That's correct. But it requires that you need to run formatter, linter, and formatter again to get a stable result which doesn't seem ideal. I see incompatibilities as everything that leads to a bad experience.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, anything that might trigger or require some command to be repeated, even if that cycle isn't infinite. In that case, we probably want to include ISC001?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should. This rule won't be conflicting with the preview style but the formatter could produce code today that conflicts with it

])
.iter_enabled()
{
if setting.linter.rules.enabled(incompatible_rule) {
incompatible_rules.push(format!("'{}'", incompatible_rule.noqa_code()));
}
}

if !incompatible_rules.is_empty() {
incompatible_rules.sort();
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.isort.split_on_trailing_comma != isort_defaults.split_on_trailing_comma {
incompatible_options.push("'isort.split_on_trailing_comma'");
}

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(", "));
}
}
}
6 changes: 5 additions & 1 deletion crates/ruff_cli/src/commands/format_stdin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ use ruff_workspace::FormatterSettings;

use crate::args::{CliOverrides, FormatArguments};
use crate::commands::format::{
format_source, FormatCommandError, FormatMode, FormatResult, FormattedSource,
format_source, warn_incompatible_formatter_settings, FormatCommandError, FormatMode,
FormatResult, FormattedSource,
};
use crate::resolve::resolve;
use crate::stdin::read_from_stdin;
Expand All @@ -27,6 +28,9 @@ pub(crate) fn format_stdin(cli: &FormatArguments, overrides: &CliOverrides) -> R
overrides,
cli.stdin_filename.as_deref(),
)?;

warn_incompatible_formatter_settings(&pyproject_config, None);

let mode = FormatMode::from_cli(cli);

if let Some(filename) = cli.stdin_filename.as_deref() {
Expand Down
86 changes: 86 additions & 0 deletions crates/ruff_cli/tests/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ fn exclude_stdin() -> Result<()> {
&ruff_toml,
r#"
extend-select = ["B", "Q"]
ignore = ["Q000", "Q001", "Q002", "Q003"]

[format]
exclude = ["generated.py"]
Expand Down Expand Up @@ -234,6 +235,7 @@ fn format_option_inheritance() -> Result<()> {
&ruff_toml,
r#"
extend = "base.toml"
extend-select = ["Q000"]

[format]
quote-style = "single"
Expand Down Expand Up @@ -277,6 +279,7 @@ if condition:

----- stderr -----
warning: `ruff format` is not yet stable, and subject to change in future versions.
warning: The following rules may cause conflicts when used with the formatter: 'Q000'. 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(())
}
Expand Down Expand Up @@ -320,6 +323,89 @@ format = "json"
Ok(())
}

#[test]
fn conflicting_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]
force-single-line = true
force-wrap-aliases = true
lines-after-imports = 0
lines-between-types = 2
split-on-trailing-comma = true
"#,
)?;

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: `ruff format` is not yet stable, and subject to change in future versions.
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.
"###);
Ok(())
}

#[test]
fn conflicting_options_stdin() -> Result<()> {
let tempdir = TempDir::new()?;
let ruff_toml = tempdir.path().join("ruff.toml");
fs::write(
&ruff_toml,
r#"
select = ["ALL"]
ignore = ["D203", "D212"]

[isort]
force-single-line = true
force-wrap-aliases = true
lines-after-imports = 0
lines-between-types = 2
split-on-trailing-comma = true
"#,
)?;

assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(["format", "--config"])
.arg(&ruff_toml)
.arg("-")
.pass_stdin(r#"
def say_hy(name: str):
print(f"Hy {name}")"#), @r###"
success: true
exit_code: 0
----- stdout -----
def say_hy(name: str):
print(f"Hy {name}")

----- stderr -----
warning: `ruff format` is not yet stable, and subject to change in future versions.
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.
"###);
Ok(())
}
#[test]
fn test_diff() {
let args = ["format", "--no-cache", "--isolated", "--diff"];
Expand Down