Skip to content
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

[flake8-pytest-style] Fix defaults when lint.flake8-pytest-style config section is empty (PT001, PT023) #13292

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dizzy57
Copy link
Contributor

@dizzy57 dizzy57 commented Sep 9, 2024

Summary

The documentation states that lint.flake8-pytest-style.*-parentheses options default to false, but if the user provides an empty [lint.flake8-pytest-style] config section, those options are suddenly set to true.

This is a bug. After #12838 changed the defaults, Flake8PytestStyleOptions::try_into_settings wasn't updated and still relies on the preview mode to set the defaults.

Reproduction

% ruff --version
ruff 0.6.4

% cat example.py
import pytest


@pytest.fixture()
def parentheses(): ...


@pytest.fixture
def no_parentheses(): ...

% ruff check --select PT001 --output-format concise example.py
example.py:4:1: PT001 [*] Use `@pytest.fixture` over `@pytest.fixture()`
Found 1 error.
[*] 1 fixable with the `--fix` option.

% echo '[lint.flake8-pytest-style]' > ruff.toml

% ruff check --select PT001 --output-format concise example.py
example.py:8:1: PT001 [*] Use `@pytest.fixture()` over `@pytest.fixture`
Found 1 error.
[*] 1 fixable with the `--fix` option.

Test Plan

Manual testing, see reproduction section.

% ./target/debug/ruff check --select PT001 --output-format concise example.py
warning: Detected debug build without --no-cache.
example.py:4:1: PT001 [*] Use `@pytest.fixture` over `@pytest.fixture()`
Found 1 error.
[*] 1 fixable with the `--fix` option.

% echo '[lint.flake8-pytest-style]' > ruff.toml

% ./target/debug/ruff check --select PT001 --output-format concise example.py
warning: Detected debug build without --no-cache.
example.py:4:1: PT001 [*] Use `@pytest.fixture` over `@pytest.fixture()`
Found 1 error.
[*] 1 fixable with the `--fix` option.

Given the fact that this was the only reference to ruff_linter::settings::types::PreviewMode in crates/ruff_workspace/src/options.rs, looks like no other settings depend on the preview mode, and it doesn't make much sense to me to write any additional tests that verify there's no interaction between preview mode and other default values.

…yle.*-parentheses` config (`PT001`, `PT023`)
@dizzy57 dizzy57 changed the title [flake8-pytest-style] Fix defaults for missing lint.flake8-pytest-style.*-parentheses configs (PT001, PT023) [flake8-pytest-style] Fix defaults when lint.flake8-pytest-style config section is empty (PT001, PT023) Sep 9, 2024
Copy link
Contributor

github-actions bot commented Sep 9, 2024

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+0 -33 violations, +0 -0 fixes in 2 projects; 52 projects unchanged)

pandas-dev/pandas (+0 -33 violations, +0 -0 fixes)

- doc/source/user_guide/style.ipynb:cell 113:31:89: E501 Line too long (90 > 88)
- doc/source/user_guide/style.ipynb:cell 113:33:89: E501 Line too long (96 > 88)
- doc/source/user_guide/style.ipynb:cell 123:3:56: E741 Ambiguous variable name: `l`
- doc/source/user_guide/style.ipynb:cell 125:2:13: C408 Unnecessary `dict` call (rewrite as a literal)
- doc/source/user_guide/style.ipynb:cell 125:4:13: C408 Unnecessary `dict` call (rewrite as a literal)
- doc/source/user_guide/style.ipynb:cell 125:6:13: C408 Unnecessary `dict` call (rewrite as a literal)
- doc/source/user_guide/style.ipynb:cell 125:8:13: C408 Unnecessary `dict` call (rewrite as a literal)
- doc/source/user_guide/style.ipynb:cell 126:1:1: NPY002 Replace legacy `np.random.seed` call with `np.random.Generator`
- doc/source/user_guide/style.ipynb:cell 126:2:1: PLW0127 Self-assignment of variable `cmap`
- doc/source/user_guide/style.ipynb:cell 126:2:8: PLW0128 Redeclared variable `cmap` in assignment
- doc/source/user_guide/style.ipynb:cell 126:3:22: NPY002 Replace legacy `np.random.randn` call with `np.random.Generator`
- doc/source/user_guide/style.ipynb:cell 128:1:22: NPY002 Replace legacy `np.random.randn` call with `np.random.Generator`
- doc/source/user_guide/style.ipynb:cell 134:1:89: E501 Line too long (93 > 88)
- doc/source/user_guide/style.ipynb:cell 141:1:89: E501 Line too long (92 > 88)
- doc/source/user_guide/style.ipynb:cell 157:1:38: F811 Redefinition of unused `f` from cell 123, line 3
- doc/source/user_guide/style.ipynb:cell 15:2:89: E501 Line too long (103 > 88)
- doc/source/user_guide/style.ipynb:cell 15:3:89: E501 Line too long (155 > 88)
- doc/source/user_guide/style.ipynb:cell 16:48:89: E501 Line too long (103 > 88)
- doc/source/user_guide/style.ipynb:cell 16:52:89: E501 Line too long (162 > 88)
... 10 additional changes omitted for rule E501
- doc/source/user_guide/style.ipynb:cell 37:1:1: NPY002 Replace legacy `np.random.seed` call with `np.random.Generator`
- doc/source/user_guide/style.ipynb:cell 37:2:20: NPY002 Replace legacy `np.random.randn` call with `np.random.Generator`
- doc/source/user_guide/style.ipynb:cell 60:1:20: NPY002 Replace legacy `np.random.randn` call with `np.random.Generator`
- doc/source/user_guide/style.ipynb:cell 6:1:27: NPY002 Replace legacy `np.random.rand` call with `np.random.Generator`
- doc/source/user_guide/style.ipynb:cell 9:1:19: NPY002 Replace legacy `np.random.randn` call with `np.random.Generator`
... 1 additional changes omitted for rule NPY002

python-trio/trio (+0 -0 violations, +0 -0 fixes)


Changes by rule (7 rules affected)

code total + violation - violation + fix - fix
E501 17 0 17 0 0
NPY002 8 0 8 0 0
C408 4 0 4 0 0
E741 1 0 1 0 0
PLW0127 1 0 1 0 0
PLW0128 1 0 1 0 0
F811 1 0 1 0 0

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry we didn't catch this, that's on us :-(

Unfortunately, however, this is will be a breaking change for some users, so we'll have to merge this as part of the 0.7 release. That should come at some point in the next few weeks, though; shouldn't be too long to wait :)

@AlexWaygood AlexWaygood added this to the v0.7 milestone Sep 9, 2024
@MichaReiser MichaReiser added breaking Breaking API change configuration Related to settings and configuration labels Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking API change configuration Related to settings and configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants