From d1aeadc009b2b8d11a55a88c87b2878e4e3b2c0f Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Mon, 1 Jul 2024 03:06:11 +0100 Subject: [PATCH] [`pytest`] Reverse `PT001` and `PT0023` defaults (#12106) ## Summary This patch inverts the defaults for [pytest-fixture-incorrect-parentheses-style (PT001)](https://docs.astral.sh/ruff/rules/pytest-fixture-incorrect-parentheses-style/) and [pytest-incorrect-mark-parentheses-style (PT003)](https://docs.astral.sh/ruff/rules/pytest-incorrect-mark-parentheses-style/) to prefer dropping superfluous parentheses. Presently, Ruff defaults to adding superfluous parentheses on pytest mark and fixture decorators for documented purpose of consistency; for example, ```diff import pytest -@pytest.mark.foo +@pytest.mark.foo() def test_bar(): ... ``` This behaviour is counter to the official pytest recommendation and diverges from the flake8-pytest-style plugin as of version 2.0.0 (see https://github.com/m-burst/flake8-pytest-style/issues/272). Seeing as either default satisfies the documented benefit of consistency across a codebase, it makes sense to change the behaviour to be consistent with pytest and the flake8 plugin as well. This change is breaking, so is gated behind preview (at least under my understanding of Ruff versioning). The implementation of this gating feature is a bit hacky, but seemed to be the least disruptive solution without performing invasive surgery on the `#[option()]` macro. Related to #8796. ### Caveat Whilst updating the documentation, I sought to reference the pytest recommendation to drop superfluous parentheses, but couldn't find any official instruction beyond it being a revealed preference within the pytest documentation code examples (as well as the linked issues from a core pytest developer). Thus, the wording of the preference is deliberately timid; it's to cohere with pytest rather than follow an explicit guidance. ## Test Plan `cargo nextest run` I also ran ```sh cargo run -p ruff -- check crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/PT001.py --no-cache --diff --select PT001 ``` and compared against it with `--preview` to verify that the default does change under preview (I also repeated this with `echo '[tool.ruff]\npreview = true' > pyproject.toml` to verify that it works with a configuration file). --------- Co-authored-by: Charlie Marsh --- .../rules/flake8_pytest_style/rules/fixture.rs | 5 +++++ .../rules/flake8_pytest_style/rules/marks.rs | 5 +++++ .../src/rules/flake8_pytest_style/settings.rs | 16 +++++++++++++++- crates/ruff_workspace/src/configuration.rs | 10 +++++++--- crates/ruff_workspace/src/options.rs | 17 +++++++++++++---- ruff.schema.json | 4 ++-- 6 files changed, 47 insertions(+), 10 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/fixture.rs b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/fixture.rs index 2c7242ae3a..9b227a46c1 100644 --- a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/fixture.rs +++ b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/fixture.rs @@ -34,6 +34,9 @@ use super::helpers::{ /// Either removing those unnecessary parentheses _or_ requiring them for all /// fixtures is fine, but it's best to be consistent. /// +/// In [preview], this rule defaults to removing unnecessary parentheses, to match +/// the behavior of official pytest projects. +/// /// ## Example /// ```python /// import pytest @@ -59,6 +62,8 @@ use super::helpers::{ /// /// ## References /// - [`pytest` documentation: API Reference: Fixtures](https://docs.pytest.org/en/latest/reference/reference.html#fixtures-api) +/// +/// [preview]: https://docs.astral.sh/ruff/preview/ #[violation] pub struct PytestFixtureIncorrectParenthesesStyle { expected: Parentheses, diff --git a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/marks.rs b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/marks.rs index cc00f2c6dd..b3beac0119 100644 --- a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/marks.rs +++ b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/marks.rs @@ -14,6 +14,9 @@ use super::helpers::get_mark_decorators; /// without parentheses, depending on the [`lint.flake8-pytest-style.mark-parentheses`] /// setting. /// +/// In [preview], this rule defaults to removing unnecessary parentheses, to match +/// the behavior of official pytest projects. +/// /// ## Why is this bad? /// If a `@pytest.mark.()` doesn't take any arguments, the parentheses are /// optional. @@ -46,6 +49,8 @@ use super::helpers::get_mark_decorators; /// /// ## References /// - [`pytest` documentation: Marks](https://docs.pytest.org/en/latest/reference/reference.html#marks) +/// +/// [preview]: https://docs.astral.sh/ruff/preview/ #[violation] pub struct PytestIncorrectMarkParenthesesStyle { mark_name: String, diff --git a/crates/ruff_linter/src/rules/flake8_pytest_style/settings.rs b/crates/ruff_linter/src/rules/flake8_pytest_style/settings.rs index 50b08c48c1..85ff1147ef 100644 --- a/crates/ruff_linter/src/rules/flake8_pytest_style/settings.rs +++ b/crates/ruff_linter/src/rules/flake8_pytest_style/settings.rs @@ -6,7 +6,7 @@ use std::fmt::Formatter; use crate::display_settings; use ruff_macros::CacheKey; -use crate::settings::types::IdentifierPattern; +use crate::settings::types::{IdentifierPattern, PreviewMode}; use super::types; @@ -49,6 +49,20 @@ impl Default for Settings { } } +impl Settings { + pub fn resolve_default(preview: PreviewMode) -> Self { + if preview.is_enabled() { + Self { + fixture_parentheses: false, + mark_parentheses: false, + ..Default::default() + } + } else { + Self::default() + } + } +} + impl fmt::Display for Settings { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { display_settings! { diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 85f7b91056..e4d1631063 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -23,7 +23,7 @@ use ruff_linter::line_width::{IndentWidth, LineLength}; use ruff_linter::registry::RuleNamespace; use ruff_linter::registry::{Rule, RuleSet, INCOMPATIBLE_CODES}; use ruff_linter::rule_selector::{PreviewOptions, Specificity}; -use ruff_linter::rules::pycodestyle; +use ruff_linter::rules::{flake8_pytest_style, pycodestyle}; use ruff_linter::settings::fix_safety_table::FixSafetyTable; use ruff_linter::settings::rule_table::RuleTable; use ruff_linter::settings::types::{ @@ -327,9 +327,13 @@ impl Configuration { .unwrap_or_default(), flake8_pytest_style: lint .flake8_pytest_style - .map(Flake8PytestStyleOptions::try_into_settings) + .map(|options| { + Flake8PytestStyleOptions::try_into_settings(options, lint_preview) + }) .transpose()? - .unwrap_or_default(), + .unwrap_or_else(|| { + flake8_pytest_style::settings::Settings::resolve_default(lint_preview) + }), flake8_quotes: lint .flake8_quotes .map(Flake8QuotesOptions::into_settings) diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 38645d3008..133ad5314c 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -24,7 +24,7 @@ use ruff_linter::rules::{ pycodestyle, pydocstyle, pyflakes, pylint, pyupgrade, }; use ruff_linter::settings::types::{ - IdentifierPattern, OutputFormat, PythonVersion, RequiredVersion, + IdentifierPattern, OutputFormat, PreviewMode, PythonVersion, RequiredVersion, }; use ruff_linter::{warn_user_once, RuleSelector}; use ruff_macros::{CombineOptions, OptionsMetadata}; @@ -1374,6 +1374,9 @@ pub struct Flake8PytestStyleOptions { /// default), `@pytest.fixture()` is valid and `@pytest.fixture` is /// invalid. If set to `false`, `@pytest.fixture` is valid and /// `@pytest.fixture()` is invalid. + /// + /// If [preview](https://docs.astral.sh/ruff/preview/) is enabled, defaults to + /// `false`. #[option( default = "true", value_type = "bool", @@ -1457,6 +1460,9 @@ pub struct Flake8PytestStyleOptions { /// default), `@pytest.mark.foo()` is valid and `@pytest.mark.foo` is /// invalid. If set to `false`, `@pytest.fixture` is valid and /// `@pytest.mark.foo()` is invalid. + /// + /// If [preview](https://docs.astral.sh/ruff/preview/) is enabled, defaults to + /// `false`. #[option( default = "true", value_type = "bool", @@ -1466,9 +1472,12 @@ pub struct Flake8PytestStyleOptions { } impl Flake8PytestStyleOptions { - pub fn try_into_settings(self) -> anyhow::Result { + pub fn try_into_settings( + self, + preview: PreviewMode, + ) -> anyhow::Result { Ok(flake8_pytest_style::settings::Settings { - fixture_parentheses: self.fixture_parentheses.unwrap_or(true), + fixture_parentheses: self.fixture_parentheses.unwrap_or(preview.is_disabled()), parametrize_names_type: self.parametrize_names_type.unwrap_or_default(), parametrize_values_type: self.parametrize_values_type.unwrap_or_default(), parametrize_values_row_type: self.parametrize_values_row_type.unwrap_or_default(), @@ -1494,7 +1503,7 @@ impl Flake8PytestStyleOptions { .transpose() .map_err(SettingsError::InvalidRaisesExtendRequireMatchFor)? .unwrap_or_default(), - mark_parentheses: self.mark_parentheses.unwrap_or(true), + mark_parentheses: self.mark_parentheses.unwrap_or(preview.is_disabled()), }) } } diff --git a/ruff.schema.json b/ruff.schema.json index cfe10e3a4c..3cb9dec724 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1093,14 +1093,14 @@ "type": "object", "properties": { "fixture-parentheses": { - "description": "Boolean flag specifying whether `@pytest.fixture()` without parameters should have parentheses. If the option is set to `true` (the default), `@pytest.fixture()` is valid and `@pytest.fixture` is invalid. If set to `false`, `@pytest.fixture` is valid and `@pytest.fixture()` is invalid.", + "description": "Boolean flag specifying whether `@pytest.fixture()` without parameters should have parentheses. If the option is set to `true` (the default), `@pytest.fixture()` is valid and `@pytest.fixture` is invalid. If set to `false`, `@pytest.fixture` is valid and `@pytest.fixture()` is invalid.\n\nIf [preview](https://docs.astral.sh/ruff/preview/) is enabled, defaults to `false`.", "type": [ "boolean", "null" ] }, "mark-parentheses": { - "description": "Boolean flag specifying whether `@pytest.mark.foo()` without parameters should have parentheses. If the option is set to `true` (the default), `@pytest.mark.foo()` is valid and `@pytest.mark.foo` is invalid. If set to `false`, `@pytest.fixture` is valid and `@pytest.mark.foo()` is invalid.", + "description": "Boolean flag specifying whether `@pytest.mark.foo()` without parameters should have parentheses. If the option is set to `true` (the default), `@pytest.mark.foo()` is valid and `@pytest.mark.foo` is invalid. If set to `false`, `@pytest.fixture` is valid and `@pytest.mark.foo()` is invalid.\n\nIf [preview](https://docs.astral.sh/ruff/preview/) is enabled, defaults to `false`.", "type": [ "boolean", "null"