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.

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

Ensure that B006 autofixes are inserted after imports #7629

Merged
merged 22 commits into from
Sep 27, 2023
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Move mutable arguments below imports and docstrings
# https://github.com/astral-sh/ruff/issues/7616


def import_module_wrong(value: dict[str, str] = {}):
import os


def import_module_with_values_wrong(value: dict[str, str] = {}):
import os

return 2


def import_modules_wrong(value: dict[str, str] = {}):
import os
import sys
import itertools


def from_import_module_wrong(value: dict[str, str] = {}):
from os import path


def from_imports_module_wrong(value: dict[str, str] = {}):
from os import path
from sys import version_info


def import_and_from_imports_module_wrong(value: dict[str, str] = {}):
import os
from sys import version_info


def import_docstring_module_wrong(value: dict[str, str] = {}):
"""Docstring"""
import os
konstin marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ mod tests {
#[test_case(Rule::MutableArgumentDefault, Path::new("B006_2.py"))]
#[test_case(Rule::MutableArgumentDefault, Path::new("B006_3.py"))]
#[test_case(Rule::MutableArgumentDefault, Path::new("B006_4.py"))]
#[test_case(Rule::MutableArgumentDefault, Path::new("B006_5.py"))]
#[test_case(Rule::NoExplicitStacklevel, Path::new("B028.py"))]
#[test_case(Rule::RaiseLiteral, Path::new("B016.py"))]
#[test_case(Rule::RaiseWithoutFromInsideExcept, Path::new("B904.py"))]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,9 @@ fn move_initialization(
indexer: &Indexer,
generator: Generator,
) -> Option<Fix> {
let mut body = function_def.body.iter();
let mut body = function_def.body.iter().peekable();

let statement = body.next()?;
let statement = body.peek()?;
if indexer.in_multi_statement_line(statement, locator) {
return None;
}
Expand All @@ -165,33 +165,37 @@ fn move_initialization(
content.push_str(stylist.line_ending().as_str());

// Indent the edit to match the body indentation.
let content = textwrap::indent(&content, indentation).to_string();

let initialization_edit = if is_docstring_stmt(statement) {
// If the first statement in the function is a docstring, insert _after_ it.
if let Some(statement) = body.next() {
// If there's a second statement, insert _before_ it, but ensure this isn't a
// multi-statement line.
if indexer.in_multi_statement_line(statement, locator) {
return None;
let mut content = textwrap::indent(&content, indentation).to_string();

// Find the position to insert the initialization after docstring and imports
let mut pos = locator.line_start(statement.start());
while let Some(statement) = body.next() {
if is_docstring_stmt(statement) {
// If the statement in the function is a docstring, insert _after_ it.
if let Some(statement) = body.peek() {
// If there's a second statement, insert _before_ it, but ensure this isn't a
// multi-statement line.
if indexer.in_multi_statement_line(statement, locator) {
return None;
}
pos = locator.line_start(statement.start());
} else if locator.full_line_end(statement.end()) == locator.text_len() {
// If the statement is at the end of the file, without a trailing newline, insert
// _after_ it with an extra newline.
content = format!("{}{}", stylist.line_ending().as_str(), content);
pos = locator.full_line_end(statement.end());
break;
} else {
// If the docstring is the only statement, insert _after_ it.
pos = locator.full_line_end(statement.end());
}
Edit::insertion(content, locator.line_start(statement.start()))
} else if locator.full_line_end(statement.end()) == locator.text_len() {
// If the statement is at the end of the file, without a trailing newline, insert
// _after_ it with an extra newline.
Edit::insertion(
format!("{}{}", stylist.line_ending().as_str(), content),
locator.full_line_end(statement.end()),
)
} else if statement.is_import_stmt() || statement.is_import_from_stmt() {
// If the statement in the function is an import, insert _after_ it.
Copy link
Member

Choose a reason for hiding this comment

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

Do you also need to handle the locator.full_line_end(statement.end()) == locator.text_len() case from above here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will take a look at it later today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should now be handled.

pos = locator.full_line_end(statement.end());
} else {
// If the docstring is the only statement, insert _after_ it.
Edit::insertion(content, locator.full_line_end(statement.end()))
}
} else {
// Otherwise, insert before the first statement.
let at = locator.line_start(statement.start());
Edit::insertion(content, at)
};

break;
};
}
let initialization_edit = Edit::insertion(content, pos);
Some(Fix::manual_edits(default_edit, [initialization_edit]))
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
---
source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
---
B006_5.py:5:49: B006 [*] Do not use mutable data structures for argument defaults
|
5 | def import_module_wrong(value: dict[str, str] = {}):
| ^^ B006
6 | import os
|
= help: Replace with `None`; initialize within function

ℹ Possible fix
2 2 | # https://github.com/astral-sh/ruff/issues/7616
3 3 |
4 4 |
5 |-def import_module_wrong(value: dict[str, str] = {}):
5 |+def import_module_wrong(value: dict[str, str] = None):
6 6 | import os
7 |+ if value is None:
8 |+ value = {}
7 9 |
8 10 |
9 11 | def import_module_with_values_wrong(value: dict[str, str] = {}):

B006_5.py:9:61: B006 [*] Do not use mutable data structures for argument defaults
|
9 | def import_module_with_values_wrong(value: dict[str, str] = {}):
| ^^ B006
10 | import os
|
= help: Replace with `None`; initialize within function

ℹ Possible fix
6 6 | import os
7 7 |
8 8 |
9 |-def import_module_with_values_wrong(value: dict[str, str] = {}):
9 |+def import_module_with_values_wrong(value: dict[str, str] = None):
10 10 | import os
11 |+ if value is None:
12 |+ value = {}
11 13 |
12 14 | return 2
13 15 |

B006_5.py:15:50: B006 [*] Do not use mutable data structures for argument defaults
|
15 | def import_modules_wrong(value: dict[str, str] = {}):
| ^^ B006
16 | import os
17 | import sys
|
= help: Replace with `None`; initialize within function

ℹ Possible fix
12 12 | return 2
13 13 |
14 14 |
15 |-def import_modules_wrong(value: dict[str, str] = {}):
15 |+def import_modules_wrong(value: dict[str, str] = None):
16 16 | import os
17 17 | import sys
18 18 | import itertools
19 |+ if value is None:
20 |+ value = {}
19 21 |
20 22 |
21 23 | def from_import_module_wrong(value: dict[str, str] = {}):

B006_5.py:21:54: B006 [*] Do not use mutable data structures for argument defaults
|
21 | def from_import_module_wrong(value: dict[str, str] = {}):
| ^^ B006
22 | from os import path
|
= help: Replace with `None`; initialize within function

ℹ Possible fix
18 18 | import itertools
19 19 |
20 20 |
21 |-def from_import_module_wrong(value: dict[str, str] = {}):
21 |+def from_import_module_wrong(value: dict[str, str] = None):
22 22 | from os import path
23 |+ if value is None:
24 |+ value = {}
23 25 |
24 26 |
25 27 | def from_imports_module_wrong(value: dict[str, str] = {}):

B006_5.py:25:55: B006 [*] Do not use mutable data structures for argument defaults
|
25 | def from_imports_module_wrong(value: dict[str, str] = {}):
| ^^ B006
26 | from os import path
27 | from sys import version_info
|
= help: Replace with `None`; initialize within function

ℹ Possible fix
22 22 | from os import path
23 23 |
24 24 |
25 |-def from_imports_module_wrong(value: dict[str, str] = {}):
25 |+def from_imports_module_wrong(value: dict[str, str] = None):
26 26 | from os import path
27 27 | from sys import version_info
28 |+ if value is None:
29 |+ value = {}
28 30 |
29 31 |
30 32 | def import_and_from_imports_module_wrong(value: dict[str, str] = {}):

B006_5.py:30:66: B006 [*] Do not use mutable data structures for argument defaults
|
30 | def import_and_from_imports_module_wrong(value: dict[str, str] = {}):
| ^^ B006
31 | import os
32 | from sys import version_info
|
= help: Replace with `None`; initialize within function

ℹ Possible fix
27 27 | from sys import version_info
28 28 |
29 29 |
30 |-def import_and_from_imports_module_wrong(value: dict[str, str] = {}):
30 |+def import_and_from_imports_module_wrong(value: dict[str, str] = None):
31 31 | import os
32 32 | from sys import version_info
33 |+ if value is None:
34 |+ value = {}
33 35 |
34 36 |
35 37 | def import_docstring_module_wrong(value: dict[str, str] = {}):

B006_5.py:35:59: B006 [*] Do not use mutable data structures for argument defaults
|
35 | def import_docstring_module_wrong(value: dict[str, str] = {}):
| ^^ B006
36 | """Docstring"""
37 | import os
|
= help: Replace with `None`; initialize within function

ℹ Possible fix
32 32 | from sys import version_info
33 33 |
34 34 |
35 |-def import_docstring_module_wrong(value: dict[str, str] = {}):
35 |+def import_docstring_module_wrong(value: dict[str, str] = None):
36 36 | """Docstring"""
37 37 | import os
38 |+ if value is None:
39 |+ value = {}