-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[ruff
] Check for non-context-manager use of pytest.raises
, pytest.warns
, and pytest.deprecated_call
(RUF061
)
#17368
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
Conversation
… + fix (`PT032`)
flake8-pytest-style
] Deprecated pytest.raises
callable form rule…flake8-pytest-style
] Deprecated pytest.raises
callable form rule + fix (PT032
)
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
RUF061 | 231 | 231 | 0 | 0 | 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
I had some code style suggestions, but the overall approach looks good to me.
With regard to the multi-statement case, I think your example in the PR summary is actually ideal, based on my reading of the pytest docs. See the first Note
box in this section:
When using pytest.raises as a context manager, it’s worthwhile to note that normal context manager rules apply and that the exception raised must be the final line in the scope of the context manager. Lines of code after that, within the scope of the context manager will not be executed. For example:
However, to be safe, we may want to avoid offering a fix if the value returned by raises
is assigned to a variable, or at least if we can see that variable being used later.
On the other hand, it's already an unsafe fix, so if my reading of the pytest docs is correct, the behavior here might be right in most cases.
Either way, we should add some test cases demonstrating whatever the behavior ends up being.
crates/ruff_linter/src/rules/flake8_pytest_style/rules/raises.rs
Outdated
Show resolved
Hide resolved
/// ## References | ||
/// - [`pytest` documentation: `pytest.raises`](https://docs.pytest.org/en/latest/reference/reference.html#pytest-raises) | ||
#[derive(ViolationMetadata)] | ||
pub(crate) struct DeprecatedPytestRaisesCallableForm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to leave the Deprecated
part out of this name since it's not actually deprecated yet, and my quick skim of the open PR making it deprecated made it sound a bit contentious to deprecate it at all. I think this can still be a useful stylistic rule even if it's not formally deprecated.
As more of a nit, I find the rest of the name a bit awkward, but I can't think of anything better that still fits within our naming conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fair, it is called the legacy
form in the docs, but they also include this statement:
Nonetheless, this form is fully supported and not deprecated in any way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we've been debating the deprecation and mostly settled on “let's try soft-deprecating and see if people complain, but meanwhile let the formatters/linters catch this”: pytest-dev/pytest#13241 (comment). The PR currently uses a PendingDeprecationWarning
FWIW.
That “legacy” form is the stdlib unittest baggage, I think. It's probably mostly met in ancient code bases. I'd call it “discouraged” instead of “deprecated”. Or, perhaps, “unintuitive” or “callback-style”.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I rename it to LegacyPytestRaises
or maybe CallablePytestRaises
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Callable seems off. Callback would be more accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about LegacyFormPytestRaises
or PytestRaisesAsFunction
?
It would match the terminolgoy used in the documentation. Other options are pytest-raises-with-func
because that's really what we detect. What I find interesting is that this form is listed in context manager form in the pytest documentation (but what I understand is that it isn't supposed to be used in a context manager position)
with raises(expected_exception: type[E] | tuple[type[E], ...], func: Callable[[...], Any], *args: Any, **kwargs: Any) → ExceptionInfo[E] as excinfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like LegacyFormPytestRaises
or maybe PytestRaisesLegacyForm
, but these all sound good to me.
Ahh, that does explain why I didn't see a signature under the Legacy
heading. I think you're right that that is the legacy form signature but with with
in front of it.
What do you think about the rule category? RUF
might make more sense just to be safe. The upstream PR still hasn't had any updates, but new rules were added in January, so it's still active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add the rule to the Ruff group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also like LegacyFormPytestRaises
, so if everyone agrees, I'll rename the rule
crates/ruff_linter/src/rules/flake8_pytest_style/rules/raises.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pytest_style/rules/raises.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pytest_style/rules/raises.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pytest_style/rules/raises.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pytest_style/rules/raises.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pytest_style/rules/raises.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pytest_style/rules/raises.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/codes.rs
Outdated
@@ -839,6 +839,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { | |||
(Flake8PytestStyle, "029") => (RuleGroup::Preview, rules::flake8_pytest_style::rules::PytestWarnsWithoutWarning), | |||
(Flake8PytestStyle, "030") => (RuleGroup::Preview, rules::flake8_pytest_style::rules::PytestWarnsTooBroad), | |||
(Flake8PytestStyle, "031") => (RuleGroup::Preview, rules::flake8_pytest_style::rules::PytestWarnsWithMultipleStatements), | |||
(Flake8PytestStyle, "032") => (RuleGroup::Preview, rules::flake8_pytest_style::rules::DeprecatedPytestRaisesCallableForm), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you said in the PR description, this may end up needing to be a ruff rule instead, but we can delay changing that until right before landing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I assign RUF103
code to the rule? Also, if the rule is renamed, is it okay for the checking code to be in flake8_pytest_style
folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like RUF60
would be the lowest unused code, but we need to double-check that there aren't any other open PRs using it.
I think we'd also want to move the implementation, tests, and snapshots to the rules/ruff
directory. It's always a bit tedious to move these around, sorry!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there are two open PRs for rules using RUF060 (and an additional closed one), so let's go for RUF061
, which I didn't see any results for.
Thanks for the review! About the multi-statement case — I initially thought it wasn’t ideal, since in the context manager form, Regarding the tests, I’ve added a multi-statement case, as well as a case where |
let indentation = leading_indentation(first_line); | ||
let mut indented = String::new(); | ||
for (idx, line) in generated.universal_newlines().enumerate() { | ||
if idx == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it guaranteed that the generator always fits the with-context on a single line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my testing, the generator always produces a single-line with
statement, even if the original pytest.raises
call was split across multiple lines. I tried two cases:
-
A very long
expected_exception
:def test(): pytest.raises( VeryLongExceptionThatExceeds80CharactersToTestWhetherGeneratedWithStatementIsOneLineOrNot, test_func, )
After applying the rule, it’s fixed to:
def test(): with pytest.raises(VeryLongExceptionThatExceeds80CharactersToTestWhetherGeneratedWithStatementIsOneLineOrNot): test_func()
Although the resulting line is not formatted according to ruff, it is still generated as a single line.
-
A multiline
match
string:def test(): pytest.raises(ValueError, test_func).match( """a b""" )
After applying the rule:
def test(): with pytest.raises(ValueError, match="""a\n b"""): test_func()
The
with
statement is produced on a single line.
I don't know if it's okay to generate potentially unformatted code, but it seems like the generator doesn't format its output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks great. I think now we just need to:
- decide on a name
- decide on a rule category
- address Micha's comment about the single line
I made some suggestions about the first two here, but I'm definitely interested in other thoughts.
Should we also handle legacy forms of |
I think it's okay to handle these separately in follow-up PRs, or even as different rules, if that sounds good to @MichaReiser. If we want to include them in this rule, it could possibly affect our name choice too. |
We should figure out the scope of this rule before landing this PR because renaming is somewhat annoying (we need to setup redirects for documentation). Whether it should be one or multiple rules normally comes down to whether there are cases where a user might only want to be warned about |
From that perspective, I'd probably lean towards one rule because it seems more consistent stylistically to move them all at once. But I'm definitely open to other ideas. I've only personally used the context manager forms of these. We can still add support for the other functions in follow-up PRs (unless they're easy to add here), but we should definitely decide on the scope and name here to avoid future churn with the name, as Micha said. |
I agree with Brent that it makes sense to combine all of these into 1 rule. I've made the following changes to the PR:
Note that unlike |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is looking great. Just a few minor nits/suggestions/questions.
call.range(), | ||
); | ||
|
||
let stmt = checker.semantic().current_statement(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let stmt = checker.semantic().current_statement(); | |
let stmt = semantic.current_statement(); |
if !has_leading_content(stmt.start(), checker.source()) | ||
&& !has_trailing_content(stmt.end(), checker.source()) | ||
{ | ||
if let Some(with_stmt) = try_fix_legacy_call(context_type, stmt, checker.semantic()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if let Some(with_stmt) = try_fix_legacy_call(context_type, stmt, checker.semantic()) { | |
if let Some(with_stmt) = try_fix_legacy_call(context_type, stmt, semantic) { |
if PytestContextType::from_expr_name(&call.func, semantic) == Some(context_type) { | ||
generate_with_statement(context_type, call, None, None, None) | ||
} else if let PytestContextType::Raises = context_type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some example Python code in a comment might help me here. It seems like this is doing something important, but all I can think is "didn't we already call PytestContextType::from_expr_name
above?" What kind of code will lead to the different branches here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This handles two cases for legacy calls:
- Direct usage:
pytest.raises(ZeroDivisionError, func, 1, b=0)
- With match method:
pytest.raises(ZeroDivisionError, func, 1, b=0).match("division by zero")
The second branch specifically looks for raises().match()
pattern which only exists for raises
(not warns
/deprecated_call
) since only raises
returns an ExceptionInfo
object with a .match()
method. We need to preserve this match condition when converting.
I have modified the comment to explain it better. There is also a test case for this in RUF061_raises.py
:
def test_error_match():
pytest.raises(ZeroDivisionError, func, 1, b=0).match("division by zero")
let expected = if let Some((name, position)) = context_type.expected_arg() { | ||
Some(legacy_call.arguments.find_argument_value(name, position)?) | ||
} else { | ||
None | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could use and_then
here (but haven't tested it):
let expected = if let Some((name, position)) = context_type.expected_arg() { | |
Some(legacy_call.arguments.find_argument_value(name, position)?) | |
} else { | |
None | |
}; | |
let expected = context_type.expected_arg().and_then(|(name, position)| legacy_call.arguments.find_argument_value(name, position)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can't use and_then
here because we need different behavior:
For raises
/warns
, the ?
operator returns early if the argument isn't found.
For deprecated_call
, we expect None
and continue.
24 24 | def test_error_lambda(): | ||
25 |- pytest.deprecated_call(lambda: warnings.warn("", DeprecationWarning)) | ||
25 |+ with pytest.deprecated_call(): | ||
26 |+ (lambda: warnings.warn("", DeprecationWarning))() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it would be kind of nice if we could unwrap the lambda
since this looks a bit awkward, but I'm not sure how tricky that would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, unwrapping the lambda would be cleaner output. For simple cases like this with no arguments, we could extract the lambda body and use it directly.
However, I am not sure if it's within the scope of the current rule. If you think it's worth addressing now though, I can expand the implementation.
fn name(self) -> &'static str { | ||
match self { | ||
Self::Raises => "raises", | ||
Self::Warns => "warns", | ||
Self::DeprecatedCall => "deprecated_call", | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do a Display
impl here? Then we could possibly store a PytestContextType
on the Violation
struct and just pass it directly to the format!
calls above.
/// - [`pytest` documentation: `pytest.warns`](https://docs.pytest.org/en/latest/reference/reference.html#pytest-warns) | ||
/// - [`pytest` documentation: `pytest.deprecated_call`](https://docs.pytest.org/en/latest/reference/reference.html#pytest-deprecated-call) | ||
#[derive(ViolationMetadata)] | ||
pub(crate) struct LegacyFormPytestRaisesWarnsDeprecatedCall { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name seems a bit unwieldy, but I'm not really coming up with something better. Is there some kind of catch-all term that covers all three?
The pytest docs for warns
actually say:
Assert that code raises a particular class of warning.
So I think it would be fairly reasonable just to call it LegacyFormPytestRaises
still. Not to say it's an ideal practice, but we do have some other rules with names that are more specific than the actual implementation, like yield-outside-function (F704), which actually covers yield
, yield from
, and even await
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's quite long. I agree that LegacyFormPytestRaises
will be cleaner. I renamed it in the latest commit.
Oh I'd completely missed this PR, happy to see progress despite my PR to I suppose I can see the logic that transforming with pytest.raises(ValueError) as excinfo:
...
assert excinfo.match("bar") into with pytest.raises(ValueError, match="bar"):
... might not be the primary responsibility of this rule, but I do think it's a shame as the latter is much cleaner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks and sorry for the delay here. I think all of my questions have been resolved.
Could you merge main
and update the title/description? GitHub isn't showing any conflicts, but I've made some changes to our diagnostic reporting that I think will actually cause problems if we don't merge first. Basically instead of Diagnostic::new
and a later checker.report_diagnostic
, you'll just need to call checker.report_diagnostic
with whatever you previously passed to Diagnostic::new
. I'm happy to help with that or take care of it if you prefer.
Other than that I think this is good to go.
Thanks! I’ve merged |
flake8-pytest-style
] Deprecated pytest.raises
callable form rule + fix (PT032
)ruff
] Check for non-contextmanager use of pytest.raises
& friends (RUF061
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! And nice work on the AtomicNodeIndex
changes, I hadn't even seen those myself yet.
Thanks! I’ve merged main and fixed the build errors from the diagnostic changes. Also updated the title earlier. It’s now LegacyFormPytestRaises as you suggested earlier. Or did you mean the PR title/description?
Yeah I meant the PR title and description, but I see you got those too. Thanks again!
ruff
] Check for non-contextmanager use of pytest.raises
& friends (RUF061
)ruff
] Check for non-context-manager use of pytest.raises
, pytest.warns
, and pytest.deprecated_call
(RUF061
)
(Just made the title a bit more verbose for inclusion directly into the release notes!) |
* main: (38 commits) [`pyupgrade`] Suppress `UP008` diagnostic if `super` symbol is not builtin (#18688) [pylint] Fix `PLW0128` to check assignment targets in square brackets and after asterisks (#18665) [`refurb`] Make the fix for `FURB163` unsafe for `log2`, `log10`, `*args`, and deleted comments (#18645) [ty] allow `T: Never` as subtype of `Never` (#18687) [ty] Use more parallelism when running corpus tests (#18711) [ty] Support `dataclasses.KW_ONLY` (#18677) [`ruff`] Check for non-context-manager use of `pytest.raises`, `pytest.warns`, and `pytest.deprecated_call` (`RUF061`) (#17368) Add syntax error when conversion flag does not immediately follow exclamation mark (#18706) [`flake8-pyi`] Fix `custom-typevar-for-self` with string annotations (`PYI019`) (#18311) Drop confusing second `*` from glob pattern example (#18709) [ty] Stabilize completions (#18650) [ty] Correctly label typeshed-sync PRs (#18702) Update Rust crate memchr to v2.7.5 (#18696) Update dependency react-resizable-panels to v3.0.3 (#18691) Update Rust crate clap to v4.5.40 (#18692) Update Rust crate libcst to v1.8.2 (#18695) Update Rust crate jiff to v0.2.15 (#18693) Update Rust crate libc to v0.2.173 (#18694) Update Rust crate syn to v2.0.103 (#18698) Update Rust crate toml to v0.8.23 (#18699) ...
This PR aims to close #16605.
Summary
This PR introduces a new rule (
RUF061
) that detects non-contextmanager usage ofpytest.raises
,pytest.warns
, andpytest.deprecated_call
. This pattern is discouraged and was proposed in flake8-pytest-style, but the corresponding PR has been open for over a month without activity.Additionally, this PR provides an unsafe fix for simple cases where the non-contextmanager form can be transformed into the context manager form. Examples of supported patterns are listed in
RUF061_raises.py
,RUF061_warns.py
, andRUF061_deprecated_call.py
test files.The more complex case from the original issue (involving two separate statements):
is getting fixed like this:
Putting match in the raises call requires multi-statement transformation, which I am not sure how to implement.
Test Plan
New test files were added to cover various usages of the non-contextmanager form of pytest.raises, warns, and deprecated_call.