Open RonnyPfannschmidt opened 10 months ago
As a user who just started adding ruff to his (somewhat big) codebase I also added config to change the weird defaults WRT parentheses because the defaults were quite weird!
Just some thoughts on your points:
PT006
: Maybe parametrize-names-type (and possibly the other settings related to parametrize as well?) should allow a list of valid types (e.g. ['csv', 'tuple']
if you don't want lists), and when applying a fix pick the first one in the list (to avoid an extra setting to set the preferred type)?PT009
: I guess this is a matter of old existing vs new/modern codebase? On the latter I absolutely want this enabled because that way someone not familiar with the nice pytest features will immediately get a linter warning if they try to use unittest-style assertions. Personally I'd simply disable it in codebases that make use of it, but I think "supporting legacy stuff" should not be a default... I'm curious, when is using the legacy assertions actually necessary? (My first thought there was "imagine if this was simply autofixable")PT021
: Would it make sense if this rule only triggered when there's just a single request.addfinalizer()
call in a fixture? I guess for most applications it's not common to have truly complex fixtures, so it nicely covers the common cases of someone simply not being aware that they can use yield
in their fixtures.Agreed, except:
PT006
- please keep this as-is, I think it is perfect and wherever I have worked people got confused about the types and for multiple prefer the tuple just until now there was no way to enforce itPT009
/PT021
/PT027
- please keep these as-is, there should be a desire to encourage the "best way" to do things as that is the entire purpose of tools like thisin that case there should also be a suggestion to drop TestCase
base class and xunit setup methods
imho PT009/PT021/PT027
should group under a unittest2pytest style thing - and perhaps take some more code off https://github.com/pytest-dev/unittest2pytest
perhaps it makes sense to trigger the pytest migration rules based on whether a pytest import is in a test file,
a very neat but expensive way would be to have "migraton" rules/fixes only apply when the line is currently being changed anyway
a very neat but expensive way would be to have "migraton" rules/fixes only apply when the line is currently being changed anyway
I would not like that in my own project; It leaves an inconsistent mess and makes PRs harder to review because now they suddenly contain no longer just the main change they are about but also 'infrastructure changes'. I prefer a separate PR for those (which can be more easily skimmed over during review)
perhaps it makes sense to trigger the pytest migration rules based on whether a pytest import is in a test file
but many test files don't contain any pytest imports if they don't use parametrization...
before i evaluated ruff i wasn't even aware of flake8-pytest-styles, as one of the pytest maintainers i'm particularly annoyed as the rules don't reflect documented and communicated best practices and the upstream doesn't seem likely to change those details
I do not use flake8-pytest-style anyway, but. I did notice that it is recommended at https://docs.pytest.org/en/stable/explanation/goodpractices.html#checking-with-flake8-pytest-style
Should it be removed?
Good point
It should
FWIW, I have no problem with shipping better pytest rules (though I haven't used pytest extensively so don't feel qualified to have good opinions on the rules themselves). In practice, it may be easiest to ship them under an entirely new pytest prefix / rule set, and mark the flake8-pytest-style
rules as deprecated, but we can make that decision later.
I'll coordinate with pytest-core to get together a official set
FYI until such time that there is an official set, for Hatch's new fmt
command I'm going to by default ignore rules PT001
, PT004
, PT005
, and PT023
, while enabling the rest.
PT001, PT023 styles are configurable by https://docs.astral.sh/ruff/settings/#flake8-pytest-style-fixture-parentheses and https://docs.astral.sh/ruff/settings/#flake8-pytest-style-mark-parentheses
Would it be possible to disable the most offending rules by default for the time being?
If you would prefer to have a large default selection of "good" rules, you can use Hatch https://hatch.pypa.io/latest/config/static-analysis/
The undesirable ones that you speak of here are excluded by default.
as one of the pytest maintainers i'm particularly annoyed as the rules don't reflect documented and communicated best practices and the upstream doesn't seem likely to change those details
I understand to a large extent what you mean. But the way I see it is that there is a difference between the possibilities of a programming language, a (testing) framework, or a library (API), and the agreements a team wants to make about a uniform usage of those assets.
As an example, I can understand that a team agrees to always use a decorator with a set of parentheses, even when there are no additional parameters to provide in a particular case, or that a team always uses the list (or tuple) form in specifying the parameter names in @pytest.mark.parametrize
instead of specifying them separated by commas in a single string. So I don't see it as a Bad Thing that these rules exist, ready to be used by any team that wants to do so.
OTOH, I didn't dive in all the particular rules, but advocating PT004
/ PT005
as a by pytest
advocated practice is incorrect IMHO.
BTW, I think a know where these two rules come from: my line of reasoning is as follows:
None
.usefixture
decorator is not possible.I assume that this might be the line of reasoning behind PT004
/ PT005
.
Unfortunately the rule is implemented in a wrong way. Instead of checking the name of the decorated function, the rule should check the name of the fixture. This could be the name of the function, but can also be different if the fixture
decorator has a name=
parameter, or when pytest_bdd.given
for example has a target_fixture=
parameter.
@charliermarsh FYI the latest version of flake8-pytest-style (v2.0.0), which was shipped on april 1st, inverted the defaults for PT001 and PT023 so that they now conform with pytest recommendations. Maybe ruff should push out a similar change soon?
If you do establish a new set of rules around pytest. A rule that warns users on the use of the @pytest.mark.usefixtur
and demands the @pytest.mark.usefixturs
. I know of a few instances where debugging this caused A LOT of work.
If you are using pants (pantsbuild.org) then rule PT004 and PT005 will really hurt. We had a session scoped fixture which for changed from django_db_modify_db_settings
into _django_db_modify_db_settings
which then caused it to be ignored and was VERY HARD to figure out later. Rule PT004 is a menace IMO.
@charliermarsh based on this detail i think a release that declares them unsafe or removes them ougth to be expedited
@RonnyPfannschmidt -- Neither of these rules have a fix though -- those changes must've been applied manually by the author.
I'd be fine to deprecate PT004 and PT005 but it will have to wait until the next minor release (we don't deprecate rules in patch releases IIRC).
Could also a rule be added. I have sometimes seen a problem with pytest.mark.usefixutres
vs. pytest.mark.usefixture
.
Could a roule be added that forbides the later?
Current plan is to deprecate PT004 and PT005 in the upcoming v0.6.
The upcoming 0.6 release deprecates PT004 and PT005 and changes the defaults for PT001 and PT023.
hi everyone,
before i evaluated ruff i wasn't even aware of flake8-pytest-styles, as one of the pytest maintainers i'm particularly annoyed as the rules don't reflect documented and communicated best practices and the upstream doesn't seem likely to change those details
PT001
should be reversed (drop brackets)PT004
is simply incorrect - public no return value fixtures should have public names for both usage in the usefixtures maker and as dependencies in other fixturesPT005
is equally incorrect - internal fixtures return values even if underscores are used - sabotage of that is not okPT006
undoes the intentionally established pattern of being able to comma separate multiple names in a single stringPT009
sabotages intentional usage of unittest (which is sometimes necessary)PT021
sabotages well established patterns for complex fixtures that setup in phases where on needs to ensure teardown but ought not to split them into multiple smaller fixturesPT023
should drop parens on marks by defaultPT024/25
should warn on any mark on a fixture as no marks are supported on fixturesPT027
also sabotages legitimate usage of unittest patterns