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

Jump to bottom

[flake8-pytest-style] Add automatic fix for pytest-parametrize-values-wrong-type (PT007) #10461

Merged
merged 6 commits into from Mar 18, 2024
Merged

[flake8-pytest-style] Add automatic fix for pytest-parametrize-values-wrong-type (PT007) #10461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7cf7515
Add fixes for PT007
autinerd Mar 18, 2024
ea85101
Convert generator-based fix to locator-based fix
autinerd Mar 18, 2024
b1f713c
Merge branch 'main' into ruff-pt007-fixes
charliermarsh Mar 18, 2024
b957996
replace `,)` with `]`
autinerd Mar 18, 2024
fcba378
Don't add , if single-element list has trailing comma already
charliermarsh Mar 18, 2024
978bdf5
Expand comment
charliermarsh Mar 18, 2024
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
1 change: 1 addition & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/PT007.py
View file
Open in desktop
Expand Up @@ -79,5 +79,6 @@ def test_single_list_of_lists(param):

@pytest.mark.parametrize("a", [1, 2])
@pytest.mark.parametrize(("b", "c"), ((3, 4), (5, 6)))
@pytest.mark.parametrize("d", [3,])
Copy link
Member

Choose a reason for hiding this comment

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

Just one bugfix: the PR was adding a trailing comma here, which led to ,,

def test_multiple_decorators(a, b, c):
pass
159 changes: 149 additions & 10 deletions crates/ruff_linter/src/rules/flake8_pytest_style/rules/parametrize.rs
View file
Open in desktop
Expand Up @@ -182,11 +182,18 @@ pub struct PytestParametrizeValuesWrongType {
}

impl Violation for PytestParametrizeValuesWrongType {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let PytestParametrizeValuesWrongType { values, row } = self;
format!("Wrong values type in `@pytest.mark.parametrize` expected `{values}` of `{row}`")
}

fn fix_title(&self) -> Option<String> {
let PytestParametrizeValuesWrongType { values, row } = self;
Some(format!("Use `{values}` of `{row}` for parameter values"))
}
}

/// ## What it does
Expand Down Expand Up @@ -493,13 +500,46 @@ fn check_values(checker: &mut Checker, names: &Expr, values: &Expr) {
match values {
Expr::List(ast::ExprList { elts, .. }) => {
if values_type != types::ParametrizeValuesType::List {
checker.diagnostics.push(Diagnostic::new(
let mut diagnostic = Diagnostic::new(
PytestParametrizeValuesWrongType {
values: values_type,
row: values_row_type,
},
values.range(),
));
);
diagnostic.set_fix({
// Determine whether the last element has a trailing comma. Single-element
// tuples _require_ a trailing comma, so this is a single-element list
// _without_ a trailing comma, we need to insert one.
let needs_trailing_comma = if let [item] = elts.as_slice() {
SimpleTokenizer::new(
checker.locator().contents(),
TextRange::new(item.end(), values.end()),
)
.all(|token| token.kind != SimpleTokenKind::Comma)
} else {
false
};

// Replace `[` with `(`.
let values_start = Edit::replacement(
"(".into(),
values.start(),
values.start() + TextSize::from(1),
);
// Replace `]` with `)` or `,)`.
let values_end = Edit::replacement(
if needs_trailing_comma {
"),".into()
} else {
")".into()
},
values.end() - TextSize::from(1),
values.end(),
);
Fix::unsafe_edits(values_start, [values_end])
});
checker.diagnostics.push(diagnostic);
}

if is_multi_named {
Expand All @@ -508,14 +548,48 @@ fn check_values(checker: &mut Checker, names: &Expr, values: &Expr) {
}
Expr::Tuple(ast::ExprTuple { elts, .. }) => {
if values_type != types::ParametrizeValuesType::Tuple {
checker.diagnostics.push(Diagnostic::new(
let mut diagnostic = Diagnostic::new(
PytestParametrizeValuesWrongType {
values: values_type,
row: values_row_type,
},
values.range(),
));
);
diagnostic.set_fix({
// Determine whether a trailing comma is present due to the _requirement_
// that a single-element tuple must have a trailing comma, e.g., `(1,)`.
//
// If the trailing comma is on its own line, we intentionally ignore it,
// since the expression is already split over multiple lines, as in:
// ```python
// @pytest.mark.parametrize(
// (
// "x",
// ),
// )
// ```
let has_trailing_comma = elts.len() == 1
&& checker.locator().up_to(values.end()).chars().rev().nth(1) == Some(',');

// Replace `(` with `[`.
let values_start = Edit::replacement(
"[".into(),
values.start(),
values.start() + TextSize::from(1),
);
// Replace `)` or `,)` with `]`.
let start = if has_trailing_comma {
values.end() - TextSize::from(2)
} else {
values.end() - TextSize::from(1)
};
let values_end = Edit::replacement("]".into(), start, values.end());

Fix::unsafe_edits(values_start, [values_end])
});
checker.diagnostics.push(diagnostic);
}

if is_multi_named {
handle_value_rows(checker, elts, values_type, values_row_type);
}
Expand Down Expand Up @@ -604,26 +678,91 @@ fn handle_value_rows(
) {
for elt in elts {
match elt {
Expr::Tuple(_) => {
Expr::Tuple(ast::ExprTuple { elts, .. }) => {
if values_row_type != types::ParametrizeValuesRowType::Tuple {
checker.diagnostics.push(Diagnostic::new(
let mut diagnostic = Diagnostic::new(
PytestParametrizeValuesWrongType {
values: values_type,
row: values_row_type,
},
elt.range(),
));
);
diagnostic.set_fix({
// Determine whether a trailing comma is present due to the _requirement_
// that a single-element tuple must have a trailing comma, e.g., `(1,)`.
//
// If the trailing comma is on its own line, we intentionally ignore it,
// since the expression is already split over multiple lines, as in:
// ```python
// @pytest.mark.parametrize(
// (
// "x",
// ),
// )
// ```
let has_trailing_comma = elts.len() == 1
&& checker.locator().up_to(elt.end()).chars().rev().nth(1) == Some(',');

// Replace `(` with `[`.
let elt_start = Edit::replacement(
"[".into(),
elt.start(),
elt.start() + TextSize::from(1),
);
// Replace `)` or `,)` with `]`.
let start = if has_trailing_comma {
elt.end() - TextSize::from(2)
} else {
elt.end() - TextSize::from(1)
};
let elt_end = Edit::replacement("]".into(), start, elt.end());
Fix::unsafe_edits(elt_start, [elt_end])
});
checker.diagnostics.push(diagnostic);
}
}
Expr::List(_) => {
Expr::List(ast::ExprList { elts, .. }) => {
if values_row_type != types::ParametrizeValuesRowType::List {
checker.diagnostics.push(Diagnostic::new(
let mut diagnostic = Diagnostic::new(
PytestParametrizeValuesWrongType {
values: values_type,
row: values_row_type,
},
elt.range(),
));
);
diagnostic.set_fix({
// Determine whether the last element has a trailing comma. Single-element
// tuples _require_ a trailing comma, so this is a single-element list
// _without_ a trailing comma, we need to insert one.
let needs_trailing_comma = if let [item] = elts.as_slice() {
SimpleTokenizer::new(
checker.locator().contents(),
TextRange::new(item.end(), elt.end()),
)
.all(|token| token.kind != SimpleTokenKind::Comma)
} else {
false
};

// Replace `[` with `(`.
let elt_start = Edit::replacement(
"(".into(),
elt.start(),
elt.start() + TextSize::from(1),
);
// Replace `]` with `)` or `,)`.
let elt_end = Edit::replacement(
if needs_trailing_comma {
",)".into()
} else {
")".into()
},
elt.end() - TextSize::from(1),
elt.end(),
);
Fix::unsafe_edits(elt_start, [elt_end])
});
checker.diagnostics.push(diagnostic);
}
}
_ => {}
Expand Down