Skip to content

Commit

Permalink
[pytest] Reverse PT001 and PT0023 defaults (#12106)
Browse files Browse the repository at this point in the history
## 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


[email protected]
[email protected]()
 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
m-burst/flake8-pytest-style#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 <[email protected]>
  • Loading branch information
tjkuson and charliermarsh authored Jul 1, 2024
1 parent d80a9d9 commit d1aeadc
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.<marker>()` doesn't take any arguments, the parentheses are
/// optional.
Expand Down Expand Up @@ -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,
Expand Down
16 changes: 15 additions & 1 deletion crates/ruff_linter/src/rules/flake8_pytest_style/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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! {
Expand Down
10 changes: 7 additions & 3 deletions crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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)
Expand Down
17 changes: 13 additions & 4 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -1466,9 +1472,12 @@ pub struct Flake8PytestStyleOptions {
}

impl Flake8PytestStyleOptions {
pub fn try_into_settings(self) -> anyhow::Result<flake8_pytest_style::settings::Settings> {
pub fn try_into_settings(
self,
preview: PreviewMode,
) -> anyhow::Result<flake8_pytest_style::settings::Settings> {
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(),
Expand All @@ -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()),
})
}
}
Expand Down
4 changes: 2 additions & 2 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit d1aeadc

Please sign in to comment.