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

Avoid invalid combination of force-sort-within-types and lines-between-types #9041

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Avoid invalid combination of force-sort-within-types and lines-between-types #9041

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
4 changes: 4 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/isort/force_sort_within_sections_lines_between.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
from a import x
import b
from c import y
import d
22 changes: 22 additions & 0 deletions crates/ruff_linter/src/rules/isort/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ fn format_import_block(
// Add a blank lines between direct and from imports.
if settings.from_first
&& lines_between_types > 0
&& !settings.force_sort_within_sections
&& line_insertion == Some(LineInsertion::Necessary)
{
for _ in 0..lines_between_types {
Expand All @@ -225,6 +226,7 @@ fn format_import_block(
// Add a blank lines between direct and from imports.
if !settings.from_first
&& lines_between_types > 0
&& !settings.force_sort_within_sections
&& line_insertion == Some(LineInsertion::Necessary)
{
for _ in 0..lines_between_types {
Expand Down Expand Up @@ -722,6 +724,26 @@ mod tests {
Ok(())
}

#[test_case(Path::new("force_sort_within_sections_lines_between.py"))]
fn force_sort_within_sections_lines_between(path: &Path) -> Result<()> {
let snapshot = format!("force_sort_within_sections_{}", path.to_string_lossy());
let mut diagnostics = test_path(
Path::new("isort").join(path).as_path(),
&LinterSettings {
isort: super::settings::Settings {
force_sort_within_sections: true,
lines_between_types: 2,
..super::settings::Settings::default()
},
src: vec![test_resource_path("fixtures/isort")],
..LinterSettings::for_rule(Rule::UnsortedImports)
},
)?;
diagnostics.sort_by_key(Ranged::start);
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test_case(Path::new("comment.py"))]
#[test_case(Path::new("comments_and_newlines.py"))]
#[test_case(Path::new("docstring.py"))]
Expand Down
4 changes: 4 additions & 0 deletions ...isort__tests__force_sort_within_sections_force_sort_within_sections_lines_between.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
source: crates/ruff_linter/src/rules/isort/mod.rs
---

11 changes: 9 additions & 2 deletions crates/ruff_workspace/src/options.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2109,6 +2109,13 @@ impl IsortOptions {
warn_user_once!("`sections` is ignored when `no-sections` is set to `true`");
}

// Verify that if `force_sort_within_sections` is `True`, then `lines_between_types` is set to `0`.
let force_sort_within_sections = self.force_sort_within_sections.unwrap_or_default();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we warn in the settings instead? Invalid configurations are otherwise still possible when using extend.

It may also be worth to aborting in that case to prevent that we change a large set of files because we ignored the setting.

Copy link
Member Author

Choose a reason for hiding this comment

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

We already ignore this in the underlying implementation, and warning here is consistent with the other validation in this method. If we want to warn on Settings, I think that would be a separate change.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, the formatter validations are done inside of settings to avoid these inconsistencies.

I think validating in settings only is sufficient. Only validating in options has the downside that we might ignore the option without a warning if one isort options comes from the extended configuration

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, the formatter validations are done inside of settings to avoid these inconsistencies.

I think validating in settings only is sufficient. Only validating in options has the downside that we might ignore the option without a warning if one isort options comes from the extended configuration

let lines_between_types = self.lines_between_types.unwrap_or_default();
if force_sort_within_sections && lines_between_types != 0 {
warn_user_once!("`lines-between-types` is ignored when `force-sort-within-sections` is set to `true`");
}

// Extract any configuration options that deal with user-defined sections.
let mut section_order: Vec<_> = self
.section_order
Expand Down Expand Up @@ -2240,7 +2247,7 @@ impl IsortOptions {
required_imports: BTreeSet::from_iter(self.required_imports.unwrap_or_default()),
combine_as_imports: self.combine_as_imports.unwrap_or(false),
force_single_line: self.force_single_line.unwrap_or(false),
force_sort_within_sections: self.force_sort_within_sections.unwrap_or(false),
force_sort_within_sections,
case_sensitive: self.case_sensitive.unwrap_or(false),
force_wrap_aliases: self.force_wrap_aliases.unwrap_or(false),
detect_same_package: self.detect_same_package.unwrap_or(true),
Expand All @@ -2263,7 +2270,7 @@ impl IsortOptions {
variables: BTreeSet::from_iter(self.variables.unwrap_or_default()),
no_lines_before: BTreeSet::from_iter(no_lines_before),
lines_after_imports: self.lines_after_imports.unwrap_or(-1),
lines_between_types: self.lines_between_types.unwrap_or_default(),
lines_between_types,
forced_separate: Vec::from_iter(self.forced_separate.unwrap_or_default()),
section_order,
no_sections,
Expand Down
Loading