Skip to content

Commit

Permalink
[flake8-pytest-style] Add automatic fix for `pytest-parametrize-val…
Browse files Browse the repository at this point in the history 
…ues-wrong-type` (`PT007`) (#10461)

## Summary

This adds automatic fixes for the `PT007` rule.

I am currently reviewing and adding Ruff rules to Home Assistant. One
rule is PT007, which has multiple hundred occurrences in the codebase,
but no automatic fix, and this is not fun to do manually, especially
because using Regexes are not really possible with this.

My knowledge of the Ruff codebase and Rust in general is not good and
this is my first PR here, so I hope it is not too bad.

One thing where I need help is: How can I have the transformed code to
be formatted automatically, instead of it being minimized as it does it
now?

## Test Plan

Using the existing fixtures and updated snapshots.
  • Loading branch information
@autinerd
autinerd committed Mar 18, 2024
1 parent ae0ff9b commit 1a2f9f0
Show file tree
Hide file tree
Showing 6 changed files with 774 additions and 70 deletions.
1 change: 1 addition & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/PT007.py
View file
Original file line number Diff line number Diff line change
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,])
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
Original file line number Diff line number Diff line change
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

0 comments on commit 1a2f9f0

Please sign in to comment.