Skip to content

[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

Merged
merged 11 commits into from
Jun 16, 2025

Conversation

twentyone212121
Copy link
Contributor

@twentyone212121 twentyone212121 commented Apr 12, 2025

This PR aims to close #16605.

Summary

This PR introduces a new rule (RUF061) that detects non-contextmanager usage of pytest.raises, pytest.warns, and pytest.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, and RUF061_deprecated_call.py test files.

The more complex case from the original issue (involving two separate statements):

excinfo = pytest.raises(ValueError, int, "hello")
assert excinfo.match("^invalid literal")

is getting fixed like this:

with pytest.raises(ValueError) as excinfo:
    int("hello")
assert excinfo.match("^invalid literal")

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.

@twentyone212121 twentyone212121 marked this pull request as draft April 12, 2025 14:08
@twentyone212121 twentyone212121 changed the title [flake8-pytest-style] Deprecated pytest.raises callable form rule… [flake8-pytest-style] Deprecated pytest.raises callable form rule + fix (PT032) Apr 12, 2025
Copy link
Contributor

github-actions bot commented Apr 12, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+231 -0 violations, +0 -0 fixes in 4 projects; 51 projects unchanged)

apache/superset (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ tests/integration_tests/datasource_tests.py:349:9: RUF061 Use context-manager form of `pytest.raises()`
+ tests/integration_tests/datasource_tests.py:497:9: RUF061 Use context-manager form of `pytest.raises()`
+ tests/integration_tests/datasource_tests.py:509:9: RUF061 Use context-manager form of `pytest.raises()`

indico/indico (+21 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ indico/core/settings/proxy_test.py:114:5: RUF061 Use context-manager form of `pytest.raises()`
+ indico/core/settings/proxy_test.py:115:5: RUF061 Use context-manager form of `pytest.raises()`
+ indico/core/settings/proxy_test.py:116:5: RUF061 Use context-manager form of `pytest.raises()`
+ indico/core/settings/proxy_test.py:117:5: RUF061 Use context-manager form of `pytest.raises()`
+ indico/core/settings/proxy_test.py:118:5: RUF061 Use context-manager form of `pytest.raises()`
+ indico/core/settings/proxy_test.py:119:5: RUF061 Use context-manager form of `pytest.raises()`
+ indico/core/settings/proxy_test.py:120:5: RUF061 Use context-manager form of `pytest.raises()`
+ indico/core/settings/proxy_test.py:170:5: RUF061 Use context-manager form of `pytest.raises()`
+ indico/core/settings/proxy_test.py:171:5: RUF061 Use context-manager form of `pytest.raises()`
+ indico/core/settings/proxy_test.py:172:5: RUF061 Use context-manager form of `pytest.raises()`
... 11 additional changes omitted for project

pytest-dev/pytest (+134 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ doc/en/example/assertion/failure_demo.py:168:9: RUF061 Use context-manager form of `pytest.raises()`
+ doc/en/example/assertion/failure_demo.py:171:9: RUF061 Use context-manager form of `pytest.raises()`
+ testing/_py/test_local.py:1001:9: RUF061 Use context-manager form of `pytest.raises()`
+ testing/_py/test_local.py:1002:9: RUF061 Use context-manager form of `pytest.raises()`
+ testing/_py/test_local.py:1102:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/_py/test_local.py:1400:9: RUF061 Use context-manager form of `pytest.raises()`
+ testing/_py/test_local.py:628:9: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_code.py:91:15: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1019:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1068:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1082:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1101:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1118:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1149:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1182:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1214:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1244:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1271:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1327:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1380:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1473:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:1568:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:225:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:243:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:254:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:276:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:297:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:315:19: RUF061 Use context-manager form of `pytest.raises()`
+ testing/code/test_excinfo.py:331:19: RUF061 Use context-manager form of `pytest.raises()`
... 105 additional changes omitted for project

astropy/astropy (+73 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ astropy/constants/tests/test_constant.py:51:5: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/constants/tests/test_constant.py:55:5: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/constants/tests/test_constant.py:58:5: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/io/fits/hdu/compressed/tests/test_compressed.py:137:9: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/io/fits/tests/test_core.py:1015:9: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/io/fits/tests/test_core.py:1016:9: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/io/fits/tests/test_core.py:1019:9: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/io/fits/tests/test_core.py:1020:9: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/io/fits/tests/test_core.py:1158:17: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/io/fits/tests/test_core.py:336:9: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/io/fits/tests/test_core.py:337:9: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/io/fits/tests/test_core.py:338:9: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/io/fits/tests/test_core.py:339:9: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/io/fits/tests/test_core.py:349:9: RUF061 Use context-manager form of `pytest.raises()`
+ astropy/io/fits/tests/test_core.py:352:9: RUF061 Use context-manager form of `pytest.raises()`
... 58 additional changes omitted for project

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
RUF061 231 231 0 0 0

@twentyone212121 twentyone212121 marked this pull request as ready for review April 12, 2025 14:36
@ntBre ntBre added the rule Implementing or modifying a lint rule label Apr 18, 2025
Copy link
Contributor

@ntBre ntBre left a 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.

/// ## References
/// - [`pytest` documentation: `pytest.raises`](https://docs.pytest.org/en/latest/reference/reference.html#pytest-raises)
#[derive(ViolationMetadata)]
pub(crate) struct DeprecatedPytestRaisesCallableForm;
Copy link
Contributor

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.

Copy link
Contributor

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.

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”.

Copy link
Contributor Author

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?

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.

Copy link
Member

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

Copy link
Contributor

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.

Copy link
Member

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.

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 also like LegacyFormPytestRaises, so if everyone agrees, I'll rename the rule

@@ -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),
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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!

Copy link
Contributor

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.

@twentyone212121
Copy link
Contributor Author

Thanks for the review!

About the multi-statement case — I initially thought it wasn’t ideal, since in the context manager form, pytest.raises accepts the match argument directly, so there's no need for a separate assertion. That said, I’m not sure whether this rule should be responsible for transforming that pattern.

Regarding the tests, I’ve added a multi-statement case, as well as a case where func is a lambda.

let indentation = leading_indentation(first_line);
let mut indented = String::new();
for (idx, line) in generated.universal_newlines().enumerate() {
if idx == 0 {
Copy link
Member

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?

Copy link
Contributor Author

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:

  1. 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.

  2. 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.

Copy link
Contributor

@ntBre ntBre left a 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.

@twentyone212121
Copy link
Contributor Author

Should we also handle legacy forms of pytest.warns and pytest.deprecated_call? They are mentioned in the PR to pytest. These functions are slightly different from pytest.raises, so I’ll need to think about how best to integrate them into the current code to avoid code duplication.

@ntBre
Copy link
Contributor

ntBre commented Apr 29, 2025

Should we also handle legacy forms of pytest.warns and pytest.deprecated_call? They are mentioned in the PR to pytest. These functions are slightly different from pytest.raises, so I’ll need to think about how best to integrate them into the current code to avoid code duplication.

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.

@MichaReiser
Copy link
Member

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 pytest.warns but not about pytest.deprecated_call or pytest.raises.

@ntBre
Copy link
Contributor

ntBre commented Apr 30, 2025

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 pytest.warns but not about pytest.deprecated_call or pytest.raises.

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.

@twentyone212121
Copy link
Contributor Author

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:

  • In the first commit, I renamed the rule and moved it to the RUF category.
  • In the second one, I've expanded the rule to cover pytest.warns and pytest.deprecated_call. Also, renamed it to LegacyFormPytestRaisesWarnsDeprecatedCall (I'm open to suggestions about the name).

Note that unlike pytest.raises, both pytest.warns and pytest.deprecated_call return the result of the func they execute, not a context. Therefore, if these functions' results are assigned to a target, we treat it as an assign target in the body rather than an optional var. I tried to minimize code duplication by handling all variants in the same functions, which may make the code appear somewhat complex.

Copy link
Contributor

@ntBre ntBre left a 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();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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) {

Comment on lines +168 to +170
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 {
Copy link
Contributor

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?

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 handles two cases for legacy calls:

  1. Direct usage: pytest.raises(ZeroDivisionError, func, 1, b=0)
  2. 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")

Comment on lines +222 to +226
let expected = if let Some((name, position)) = context_type.expected_arg() {
Some(legacy_call.arguments.find_argument_value(name, position)?)
} else {
None
};
Copy link
Contributor

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):

Suggested change
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));

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 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))()
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Comment on lines 100 to 106
fn name(self) -> &'static str {
match self {
Self::Raises => "raises",
Self::Warns => "warns",
Self::DeprecatedCall => "deprecated_call",
}
}
Copy link
Contributor

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@jakkdl
Copy link

jakkdl commented May 26, 2025

Oh I'd completely missed this PR, happy to see progress despite my PR to flake8-pytest-style going stale. 🚀

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.

Copy link
Contributor

@ntBre ntBre left a 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.

@ntBre ntBre added the preview Related to preview mode features label Jun 16, 2025
@ntBre ntBre added the preview Related to preview mode features label Jun 16, 2025
@twentyone212121
Copy link
Contributor Author

twentyone212121 commented Jun 16, 2025

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?

@twentyone212121 twentyone212121 changed the title [flake8-pytest-style] Deprecated pytest.raises callable form rule + fix (PT032) [ruff] Check for non-contextmanager use of pytest.raises & friends (RUF061) Jun 16, 2025
Copy link
Contributor

@ntBre ntBre left a 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!

@ntBre ntBre changed the title [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) Jun 16, 2025
@ntBre
Copy link
Contributor

ntBre commented Jun 16, 2025

(Just made the title a bit more verbose for inclusion directly into the release notes!)

@ntBre ntBre merged commit c3aa965 into astral-sh:main Jun 16, 2025
34 checks passed
dcreager added a commit that referenced this pull request Jun 16, 2025
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autofix non-contextmanager use of pytest.raises & friends
5 participants