From 727c9c4f49d8e15ec4ec49d986109d1191e8c07f Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Sat, 29 Jun 2024 21:02:52 +0100 Subject: [PATCH 1/4] Reverse PT001 and PT0023 defaults The defaults for PT001 and PT003 are counter to the official pytest recommendation and diverge from the flake8-pytest-style plugin. This change is breaking so is gated behind preview. --- .../rules/flake8_pytest_style/rules/fixture.rs | 5 +++++ .../src/rules/flake8_pytest_style/rules/marks.rs | 5 +++++ .../src/rules/flake8_pytest_style/settings.rs | 14 ++++++++++++++ crates/ruff_workspace/src/configuration.rs | 15 ++++++++++++--- crates/ruff_workspace/src/options.rs | 13 ++++++++++--- ruff.schema.json | 4 ++-- 6 files changed, 48 insertions(+), 8 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 2c7242ae3af84..bf251c800a189 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. /// +/// Whilst in [preview], this rule defaults to removing unnecessary parentheses +/// to cohere better with 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 cc00f2c6ddba4..f6b680ff57786 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. /// +/// Whilst in [preview], this rule defaults to removing unnecessary parentheses +/// to cohere better with 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 50b08c48c1f91..5eca6026f4d6a 100644 --- a/crates/ruff_linter/src/rules/flake8_pytest_style/settings.rs +++ b/crates/ruff_linter/src/rules/flake8_pytest_style/settings.rs @@ -49,6 +49,20 @@ impl Default for Settings { } } +impl Settings { + pub fn resolve_default(preview: bool) -> Self { + if preview { + 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 85f7b91056649..040f9e78150c9 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,18 @@ 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.is_enabled(), + ) + }) .transpose()? - .unwrap_or_default(), + .unwrap_or_else(|| { + flake8_pytest_style::settings::Settings::resolve_default( + lint_preview.is_enabled(), + ) + }), 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 38645d3008069..a80acd381e884 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -1374,6 +1374,8 @@ 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 +1459,8 @@ 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 +1470,12 @@ pub struct Flake8PytestStyleOptions { } impl Flake8PytestStyleOptions { - pub fn try_into_settings(self) -> anyhow::Result { + pub fn try_into_settings( + self, + preview: bool, + ) -> anyhow::Result { Ok(flake8_pytest_style::settings::Settings { - fixture_parentheses: self.fixture_parentheses.unwrap_or(true), + fixture_parentheses: self.fixture_parentheses.unwrap_or(!preview), 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 +1501,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), }) } } diff --git a/ruff.schema.json b/ruff.schema.json index 4c77a7dd895ce..f49fffef4554b 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" From 530c0841901d4cacf1357adc5319c0a9ae2709b2 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Sun, 30 Jun 2024 17:02:54 +0100 Subject: [PATCH 2/4] Spell pytest lowercase --- .../ruff_linter/src/rules/flake8_pytest_style/rules/fixture.rs | 2 +- crates/ruff_linter/src/rules/flake8_pytest_style/rules/marks.rs | 2 +- 2 files changed, 2 insertions(+), 2 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 bf251c800a189..34f689b794625 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 @@ -35,7 +35,7 @@ use super::helpers::{ /// fixtures is fine, but it's best to be consistent. /// /// Whilst in [preview], this rule defaults to removing unnecessary parentheses -/// to cohere better with official Pytest projects. +/// to cohere better with official pytest projects. /// /// ## Example /// ```python 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 f6b680ff57786..aa2a98236e2b9 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 @@ -15,7 +15,7 @@ use super::helpers::get_mark_decorators; /// setting. /// /// Whilst in [preview], this rule defaults to removing unnecessary parentheses -/// to cohere better with official Pytest projects. +/// to cohere better with official pytest projects. /// /// ## Why is this bad? /// If a `@pytest.mark.()` doesn't take any arguments, the parentheses are From 1f03aa9b01e92fb9affd599cf707d8b42c51bc12 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 30 Jun 2024 21:57:35 -0400 Subject: [PATCH 3/4] Pass PreviewMode --- .../src/rules/flake8_pytest_style/rules/fixture.rs | 4 ++-- .../src/rules/flake8_pytest_style/rules/marks.rs | 4 ++-- .../src/rules/flake8_pytest_style/settings.rs | 6 +++--- crates/ruff_workspace/src/configuration.rs | 9 ++------- crates/ruff_workspace/src/options.rs | 14 ++++++++------ 5 files changed, 17 insertions(+), 20 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 34f689b794625..9b227a46c10ff 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,8 +34,8 @@ use super::helpers::{ /// Either removing those unnecessary parentheses _or_ requiring them for all /// fixtures is fine, but it's best to be consistent. /// -/// Whilst in [preview], this rule defaults to removing unnecessary parentheses -/// to cohere better with official pytest projects. +/// In [preview], this rule defaults to removing unnecessary parentheses, to match +/// the behavior of official pytest projects. /// /// ## Example /// ```python 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 aa2a98236e2b9..b3beac0119227 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,8 +14,8 @@ use super::helpers::get_mark_decorators; /// without parentheses, depending on the [`lint.flake8-pytest-style.mark-parentheses`] /// setting. /// -/// Whilst in [preview], this rule defaults to removing unnecessary parentheses -/// to cohere better with official pytest projects. +/// 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 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 5eca6026f4d6a..85ff1147efbb2 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; @@ -50,8 +50,8 @@ impl Default for Settings { } impl Settings { - pub fn resolve_default(preview: bool) -> Self { - if preview { + pub fn resolve_default(preview: PreviewMode) -> Self { + if preview.is_enabled() { Self { fixture_parentheses: false, mark_parentheses: false, diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 040f9e78150c9..e4d16310639c2 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -328,16 +328,11 @@ impl Configuration { flake8_pytest_style: lint .flake8_pytest_style .map(|options| { - Flake8PytestStyleOptions::try_into_settings( - options, - lint_preview.is_enabled(), - ) + Flake8PytestStyleOptions::try_into_settings(options, lint_preview) }) .transpose()? .unwrap_or_else(|| { - flake8_pytest_style::settings::Settings::resolve_default( - lint_preview.is_enabled(), - ) + flake8_pytest_style::settings::Settings::resolve_default(lint_preview) }), flake8_quotes: lint .flake8_quotes diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index a80acd381e884..133ad5314c859 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}; @@ -1375,7 +1375,8 @@ pub struct Flake8PytestStyleOptions { /// 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. + /// If [preview](https://docs.astral.sh/ruff/preview/) is enabled, defaults to + /// `false`. #[option( default = "true", value_type = "bool", @@ -1460,7 +1461,8 @@ pub struct Flake8PytestStyleOptions { /// 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. + /// If [preview](https://docs.astral.sh/ruff/preview/) is enabled, defaults to + /// `false`. #[option( default = "true", value_type = "bool", @@ -1472,10 +1474,10 @@ pub struct Flake8PytestStyleOptions { impl Flake8PytestStyleOptions { pub fn try_into_settings( self, - preview: bool, + preview: PreviewMode, ) -> anyhow::Result { Ok(flake8_pytest_style::settings::Settings { - fixture_parentheses: self.fixture_parentheses.unwrap_or(!preview), + 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(), @@ -1501,7 +1503,7 @@ impl Flake8PytestStyleOptions { .transpose() .map_err(SettingsError::InvalidRaisesExtendRequireMatchFor)? .unwrap_or_default(), - mark_parentheses: self.mark_parentheses.unwrap_or(!preview), + mark_parentheses: self.mark_parentheses.unwrap_or(preview.is_disabled()), }) } } From ab04b17c4ae438bf1787681e51f9e65b7ef9fc2b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 30 Jun 2024 22:01:51 -0400 Subject: [PATCH 4/4] Update schema --- ruff.schema.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ruff.schema.json b/ruff.schema.json index f49fffef4554b..f8c7ae391e57e 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.\n\nIf [preview](https://docs.astral.sh/ruff/preview/) is enabled, defaults to false.", + "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.\n\nIf [preview](https://docs.astral.sh/ruff/preview/) is enabled, defaults to false.", + "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"