From 1e2df07544fa955342d9906e70ae5d9fca28bd97 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 12 Apr 2023 11:28:25 -0400 Subject: [PATCH] Use identifier range for pytest rules (#3948) --- .../flake8_pytest_style/rules/fixture.rs | 30 +++++++-------- ...es__flake8_pytest_style__tests__PT004.snap | 22 +++++------ ...es__flake8_pytest_style__tests__PT005.snap | 37 +++++++++---------- 3 files changed, 43 insertions(+), 46 deletions(-) diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/fixture.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/fixture.rs index b4a2615d5e..b5c3221af9 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/fixture.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/fixture.rs @@ -8,8 +8,8 @@ use ruff_python_ast::call_path::collect_call_path; use ruff_python_ast::helpers::collect_arg_names; use ruff_python_ast::source_code::Locator; use ruff_python_ast::types::Range; -use ruff_python_ast::visitor; use ruff_python_ast::visitor::Visitor; +use ruff_python_ast::{helpers, visitor}; use crate::autofix::actions::remove_argument; use crate::checkers::ast::Checker; @@ -310,7 +310,7 @@ fn check_fixture_decorator(checker: &mut Checker, func_name: &str, decorator: &E { let scope_keyword = keywords .iter() - .find(|kw| kw.node.arg == Some("scope".to_string())); + .find(|kw| kw.node.arg.as_ref().map_or(false, |arg| arg == "scope")); if let Some(scope_keyword) = scope_keyword { if keyword_is_literal(scope_keyword, "function") { @@ -352,7 +352,7 @@ fn check_fixture_decorator(checker: &mut Checker, func_name: &str, decorator: &E } /// PT004, PT005, PT022 -fn check_fixture_returns(checker: &mut Checker, func: &Stmt, func_name: &str, body: &[Stmt]) { +fn check_fixture_returns(checker: &mut Checker, stmt: &Stmt, name: &str, body: &[Stmt]) { let mut visitor = SkipFunctionsVisitor::default(); for stmt in body { @@ -364,13 +364,13 @@ fn check_fixture_returns(checker: &mut Checker, func: &Stmt, func_name: &str, bo .rules .enabled(Rule::PytestIncorrectFixtureNameUnderscore) && visitor.has_return_with_value - && func_name.starts_with('_') + && name.starts_with('_') { checker.diagnostics.push(Diagnostic::new( PytestIncorrectFixtureNameUnderscore { - function: func_name.to_string(), + function: name.to_string(), }, - Range::from(func), + helpers::identifier_range(stmt, checker.locator), )); } else if checker .settings @@ -378,13 +378,13 @@ fn check_fixture_returns(checker: &mut Checker, func: &Stmt, func_name: &str, bo .enabled(Rule::PytestMissingFixtureNameUnderscore) && !visitor.has_return_with_value && !visitor.has_yield_from - && !func_name.starts_with('_') + && !name.starts_with('_') { checker.diagnostics.push(Diagnostic::new( PytestMissingFixtureNameUnderscore { - function: func_name.to_string(), + function: name.to_string(), }, - Range::from(func), + helpers::identifier_range(stmt, checker.locator), )); } @@ -399,7 +399,7 @@ fn check_fixture_returns(checker: &mut Checker, func: &Stmt, func_name: &str, bo if visitor.yield_statements.len() == 1 { let mut diagnostic = Diagnostic::new( PytestUselessYieldFixture { - name: func_name.to_string(), + name: name.to_string(), }, Range::from(stmt), ); @@ -508,8 +508,8 @@ fn check_fixture_marks(checker: &mut Checker, decorators: &[Expr]) { pub fn fixture( checker: &mut Checker, - func: &Stmt, - func_name: &str, + stmt: &Stmt, + name: &str, args: &Arguments, decorators: &[Expr], body: &[Stmt], @@ -529,7 +529,7 @@ pub fn fixture( .rules .enabled(Rule::PytestExtraneousScopeFunction) { - check_fixture_decorator(checker, func_name, decorator); + check_fixture_decorator(checker, name, decorator); } if checker @@ -555,7 +555,7 @@ pub fn fixture( .enabled(Rule::PytestUselessYieldFixture)) && !has_abstractmethod_decorator(decorators, checker) { - check_fixture_returns(checker, func, func_name, body); + check_fixture_returns(checker, stmt, name, body); } if checker @@ -583,7 +583,7 @@ pub fn fixture( .settings .rules .enabled(Rule::PytestFixtureParamWithoutValue) - && func_name.starts_with("test_") + && name.starts_with("test_") { check_test_function_args(checker, args); } diff --git a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT004.snap b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT004.snap index 21c3bb62df..589bbd8f12 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT004.snap +++ b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT004.snap @@ -1,21 +1,21 @@ --- source: crates/ruff/src/rules/flake8_pytest_style/mod.rs --- -PT004.py:51:1: PT004 Fixture `patch_something` does not return anything, add leading underscore +PT004.py:51:5: PT004 Fixture `patch_something` does not return anything, add leading underscore | -51 | @pytest.fixture() -52 | / def patch_something(mocker): # Error simple -53 | | mocker.patch("some.thing") - | |______________________________^ PT004 +51 | @pytest.fixture() +52 | def patch_something(mocker): # Error simple + | ^^^^^^^^^^^^^^^ PT004 +53 | mocker.patch("some.thing") | -PT004.py:56:1: PT004 Fixture `activate_context` does not return anything, add leading underscore +PT004.py:56:5: PT004 Fixture `activate_context` does not return anything, add leading underscore | -56 | @pytest.fixture() -57 | / def activate_context(): # Error with yield -58 | | with context: -59 | | yield - | |_____________^ PT004 +56 | @pytest.fixture() +57 | def activate_context(): # Error with yield + | ^^^^^^^^^^^^^^^^ PT004 +58 | with context: +59 | yield | diff --git a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT005.snap b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT005.snap index 5b3e5d58b4..1b7c0a265a 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT005.snap +++ b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT005.snap @@ -1,33 +1,30 @@ --- source: crates/ruff/src/rules/flake8_pytest_style/mod.rs --- -PT005.py:41:1: PT005 Fixture `_my_fixture` returns a value, remove leading underscore +PT005.py:41:5: PT005 Fixture `_my_fixture` returns a value, remove leading underscore | -41 | @pytest.fixture() -42 | / def _my_fixture(mocker): # Error with return -43 | | return 0 - | |____________^ PT005 +41 | @pytest.fixture() +42 | def _my_fixture(mocker): # Error with return + | ^^^^^^^^^^^ PT005 +43 | return 0 | -PT005.py:46:1: PT005 Fixture `_activate_context` returns a value, remove leading underscore +PT005.py:46:5: PT005 Fixture `_activate_context` returns a value, remove leading underscore | -46 | @pytest.fixture() -47 | / def _activate_context(): # Error with yield -48 | | with get_context() as context: -49 | | yield context - | |_____________________^ PT005 +46 | @pytest.fixture() +47 | def _activate_context(): # Error with yield + | ^^^^^^^^^^^^^^^^^ PT005 +48 | with get_context() as context: +49 | yield context | -PT005.py:52:1: PT005 Fixture `_activate_context` returns a value, remove leading underscore +PT005.py:52:5: PT005 Fixture `_activate_context` returns a value, remove leading underscore | -52 | @pytest.fixture() -53 | / def _activate_context(): # Error with conditional yield from -54 | | if some_condition: -55 | | with get_context() as context: -56 | | yield context -57 | | else: -58 | | yield from other_context() - | |__________________________________^ PT005 +52 | @pytest.fixture() +53 | def _activate_context(): # Error with conditional yield from + | ^^^^^^^^^^^^^^^^^ PT005 +54 | if some_condition: +55 | with get_context() as context: |