Skip to content

Commit

Permalink
Split within not for PT018
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Sep 5, 2023
1 parent 955501f commit 480e604
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 10 deletions.
18 changes: 18 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pytest_style/PT018.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,21 @@ def test_multiline():

x = 1; \
assert something and something_else


# Regression test for: https://github.com/astral-sh/ruff/issues/7143
def test_parenthesized_not():
assert not (
self.find_graph_output(node.output[0])
or self.find_graph_input(node.input[0])
or self.find_graph_output(node.input[0])
)

assert (not (
self.find_graph_output(node.output[0])
or self.find_graph_input(node.input[0])
or self.find_graph_output(node.input[0])
))

assert (not self.find_graph_output(node.output[0]) or
self.find_graph_input(node.input[0]))
25 changes: 16 additions & 9 deletions crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::borrow::Cow;
use std::ops::Deref;

use anyhow::Result;
use anyhow::{bail, Context};
Expand Down Expand Up @@ -598,7 +599,7 @@ fn negate<'a>(expression: &Expression<'a>) -> Expression<'a> {
/// assert (b ==
/// "")
/// ```
fn parenthesize<'a>(expression: Expression<'a>, parent: &Expression<'a>) -> Expression<'a> {
fn parenthesize<'a>(expression: &Expression<'a>, parent: &Expression<'a>) -> Expression<'a> {
if matches!(
expression,
Expression::Comparison(_)
Expand Down Expand Up @@ -626,10 +627,10 @@ fn parenthesize<'a>(expression: Expression<'a>, parent: &Expression<'a>) -> Expr
| Expression::NamedExpr(_)
) {
if let (Some(left), Some(right)) = (parent.lpar().first(), parent.rpar().first()) {
return expression.with_parens(left.clone(), right.clone());
return expression.clone().with_parens(left.clone(), right.clone());
}
}
expression
expression.clone()
}

/// Replace composite condition `assert a == "hello" and b == "world"` with two statements
Expand Down Expand Up @@ -685,10 +686,16 @@ fn fix_composite_condition(stmt: &Stmt, locator: &Locator, stylist: &Stylist) ->
match &assert_statement.test {
Expression::UnaryOperation(op) => {
if matches!(op.operator, libcst_native::UnaryOp::Not { .. }) {
if let Expression::BooleanOperation(op) = &*op.expression {
if matches!(op.operator, BooleanOp::Or { .. }) {
conditions.push(parenthesize(negate(&op.left), &assert_statement.test));
conditions.push(parenthesize(negate(&op.right), &assert_statement.test));
if let Expression::BooleanOperation(boolean_operation) = &*op.expression {
if matches!(boolean_operation.operator, BooleanOp::Or { .. }) {
conditions.push(negate(&parenthesize(
&*boolean_operation.left,
&*op.expression,
)));
conditions.push(negate(&parenthesize(
&*boolean_operation.right.deref(),
&*op.expression,
)));
} else {
bail!("Expected assert statement to be a composite condition");
}
Expand All @@ -699,8 +706,8 @@ fn fix_composite_condition(stmt: &Stmt, locator: &Locator, stylist: &Stylist) ->
}
Expression::BooleanOperation(op) => {
if matches!(op.operator, BooleanOp::And { .. }) {
conditions.push(parenthesize(*op.left.clone(), &assert_statement.test));
conditions.push(parenthesize(*op.right.clone(), &assert_statement.test));
conditions.push(parenthesize(&*op.left, &assert_statement.test));
conditions.push(parenthesize(&*op.right, &assert_statement.test));
} else {
bail!("Expected assert statement to be a composite condition");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ PT018.py:20:5: PT018 [*] Assertion should be broken down into multiple parts
18 18 | assert not something and something_else
19 19 | assert not (something or something_else)
20 |- assert not (something or something_else or something_third)
20 |+ assert not something or something_else
20 |+ assert not (something or something_else)
21 |+ assert not something_third
21 22 | assert something and something_else == """error
22 23 | message
Expand Down Expand Up @@ -351,4 +351,67 @@ PT018.py:54:9: PT018 Assertion should be broken down into multiple parts
|
= help: Break down assertion into multiple parts

PT018.py:59:5: PT018 [*] Assertion should be broken down into multiple parts
|
57 | # Regression test for: https://github.com/astral-sh/ruff/issues/7143
58 | def test_parenthesized_not():
59 | assert not (
| _____^
60 | | self.find_graph_output(node.output[0])
61 | | or self.find_graph_input(node.input[0])
62 | | or self.find_graph_output(node.input[0])
63 | | )
| |_____^ PT018
64 |
65 | assert (not (
|
= help: Break down assertion into multiple parts

Suggested fix
59 59 | assert not (
60 60 | self.find_graph_output(node.output[0])
61 61 | or self.find_graph_input(node.input[0])
62 |- or self.find_graph_output(node.input[0])
63 62 | )
63 |+ assert not (
64 |+ self.find_graph_output(node.input[0])
65 |+ )
64 66 |
65 67 | assert (not (
66 68 | self.find_graph_output(node.output[0])

PT018.py:65:5: PT018 [*] Assertion should be broken down into multiple parts
|
63 | )
64 |
65 | assert (not (
| _____^
66 | | self.find_graph_output(node.output[0])
67 | | or self.find_graph_input(node.input[0])
68 | | or self.find_graph_output(node.input[0])
69 | | ))
| |______^ PT018
70 |
71 | assert (not self.find_graph_output(node.output[0]) or
|
= help: Break down assertion into multiple parts

Suggested fix
62 62 | or self.find_graph_output(node.input[0])
63 63 | )
64 64 |
65 |- assert (not (
65 |+ assert not (
66 66 | self.find_graph_output(node.output[0])
67 67 | or self.find_graph_input(node.input[0])
68 |- or self.find_graph_output(node.input[0])
69 |- ))
68 |+ )
69 |+ assert not (
70 |+ self.find_graph_output(node.input[0])
71 |+ )
70 72 |
71 73 | assert (not self.find_graph_output(node.output[0]) or
72 74 | self.find_graph_input(node.input[0]))


0 comments on commit 480e604

Please sign in to comment.