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

reverse pytest-mark-no-parenthese and pytest-fixture-no-parentheses #272

Closed
RonnyPfannschmidt opened this issue Nov 18, 2023 · 6 comments · Fixed by #280
Closed

reverse pytest-mark-no-parenthese and pytest-fixture-no-parentheses #272

RonnyPfannschmidt opened this issue Nov 18, 2023 · 6 comments · Fixed by #280
Labels
enhancement New feature or request

Comments

@RonnyPfannschmidt
Copy link

the general suggestion we use in the pytest docs and in most related projects is dropping the empty parentheses if feasible

@RonnyPfannschmidt RonnyPfannschmidt added the enhancement New feature or request label Nov 18, 2023
@m-burst
Copy link
Owner

m-burst commented Nov 20, 2023

Hi @RonnyPfannschmidt,

I understand that the default behaviour of PT001 and PT023 goes against the code style shown in pytest docs.
Unfortunately, changing the defaults now would be a breaking change, which I would prefer to avoid.
The behaviour for both rules can be configured in flake8 config.

@RonnyPfannschmidt
Copy link
Author

from my pov thats a problem

just for my usages i'd have to add that rule to >100 git repos to ensure consistency or avoid the tool

ruff currently mirrors the behaviour but plans to witch around

@m-burst
Copy link
Owner

m-burst commented Nov 20, 2023

i'd have to add that rule to >100 git repos to ensure consistency

May I clarify what you mean by 'adding the rule'?
If you are planning to add the plugin to your dev dependencies, you can configure it at the same time, so it will enforce your preferred code style.

ruff currently mirrors the behaviour but plans to witch around

Is there a corresponding issue in ruff?

@RonnyPfannschmidt
Copy link
Author

So far none of the git repos where I intend to add this has a costom flake8 config, I'm relatively strictly against adding one as it's a starting point that allows people inconsistency

There's not yet a issue in ruff do for the actual detail

They plan a larger reorganize off the rules away from replicating other ecosystem plugins and discussions about this issue there indicates that they would also prefer a default rule that's the reverse of the replica of this plugin

@Mogost
Copy link

Mogost commented Nov 28, 2023

I think we need to make this breaking change. The default configuration should promote the best variant. Also it not a big breaking change because it's configurable.

@m-burst
Copy link
Owner

m-burst commented Apr 1, 2024

I've just released v2.0.0 with this change.

charliermarsh added a commit to astral-sh/ruff that referenced this issue Jul 1, 2024
## 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants