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

importer: skip whitespace between comments at start of file #6523

Merged
merged 1 commit into from
Aug 12, 2023
Merged

importer: skip whitespace between comments at start of file #6523

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
6 changes: 6 additions & 0 deletions crates/ruff/resources/test/fixtures/isort/required_imports/comments_and_newlines.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#!/usr/bin/env python3
# A copyright notice could go here

# A linter directive could go here

x = 1
8 changes: 6 additions & 2 deletions crates/ruff/src/importer/insertion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,13 @@ impl<'a> Insertion<'a> {
TextSize::default()
};

// Skip over commented lines.
// Skip over commented lines, with whitespace separation.
for line in UniversalNewlineIterator::with_offset(locator.after(location), location) {
if line.trim_whitespace_start().starts_with('#') {
let trimmed_line = line.trim_whitespace_start();
if trimmed_line.is_empty() {
continue;
}
if trimmed_line.starts_with('#') {
Copy link
Member

Choose a reason for hiding this comment

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

Given:

# This is a comment

# This is a comment about this specific import
from sys import stdout

I guess we'd now do:

# This is a comment

# This is a comment about this specific import
from __future__ import annotations
from sys import stdout

I guess we arguably already had that problem, since given:

# This is a comment about this specific import
from sys import stdout

We already do:

# This is a comment about this specific import
from __future__ import annotations
from sys import stdout

I suppose we could modify the logic to only skip comments if the comments are followed by a blank line but that may be surprising -- I'm not sure. What do you think?

Copy link
Contributor Author

@durumu durumu Aug 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good point. For what it's worth, isort -a "from __future__ import annotations" seems to do the same thing:

# This is a comment

# Comment about this import
from foo import bar

turns into:

# This is a comment

# Comment about this import
from __future__ import annotations

from foo import bar

That makes me inclined to think this case isn't too valuable to support. If we do want to support it, though, I do think your proposal of skipping comments if the comments are followed by a blank line is pretty reasonable and intuitive.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for looking into that. If isort has this behavior, then it seems reasonable to me to proceed as-is.

location = line.full_end();
} else {
break;
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/isort/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,6 +762,7 @@ mod tests {
}

#[test_case(Path::new("comment.py"))]
#[test_case(Path::new("comments_and_newlines.py"))]
#[test_case(Path::new("docstring.py"))]
#[test_case(Path::new("docstring.pyi"))]
#[test_case(Path::new("docstring_only.py"))]
Expand Down Expand Up @@ -791,6 +792,7 @@ mod tests {
}

#[test_case(Path::new("comment.py"))]
#[test_case(Path::new("comments_and_newlines.py"))]
#[test_case(Path::new("docstring.py"))]
#[test_case(Path::new("docstring.pyi"))]
#[test_case(Path::new("docstring_only.py"))]
Expand Down
20 changes: 20 additions & 0 deletions .../isort/snapshots/ruff__rules__isort__tests__required_import_comments_and_newlines.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
source: crates/ruff/src/rules/isort/mod.rs
---
comments_and_newlines.py:1:1: I002 [*] Missing required import: `from __future__ import annotations`
|
1 | #!/usr/bin/env python3
| I002
2 | # A copyright notice could go here
|
= help: Insert required import: `from future import annotations`

ℹ Fix
2 2 | # A copyright notice could go here
3 3 |
4 4 | # A linter directive could go here
5 |+from __future__ import annotations
5 6 |
6 7 | x = 1


20 changes: 20 additions & 0 deletions ...shots/ruff__rules__isort__tests__required_import_with_alias_comments_and_newlines.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
source: crates/ruff/src/rules/isort/mod.rs
---
comments_and_newlines.py:1:1: I002 [*] Missing required import: `from __future__ import annotations as _annotations`
|
1 | #!/usr/bin/env python3
| I002
2 | # A copyright notice could go here
|
= help: Insert required import: `from future import annotations as _annotations`

ℹ Fix
2 2 | # A copyright notice could go here
3 3 |
4 4 | # A linter directive could go here
5 |+from __future__ import annotations as _annotations
5 6 |
6 7 | x = 1