Simplify positional arguments compatibility code in pytest.fixture()#6999
Conversation
| warnings.warn(FIXTURE_POSITIONAL_ARGUMENTS, stacklevel=2) | ||
| # End backward compatiblity. | ||
|
|
||
| if fixture_function and params is None and autouse is False: |
There was a problem hiding this comment.
BTW, I'm interested to know why the params is None and autouse is False condition exists here, if anyone knows... Seems weird to just ignore fixture_function if params or autouse are specified?
There was a problem hiding this comment.
Judging from the # direct decoration comment, that's to support
@pytest.fixture
def func():
....rather than
@pytest.fixture(...)
def func():
...so there's nothing being ignored, because there are no arguments given at all in that case.
There was a problem hiding this comment.
Right, the if fixture_function part I understand, what I'm having trouble figuring out is the and params is None and autouse is False part.
The reason I care about it is that I am working on adding type-annotating pytest.fixture(). Without this extra params condition, it is straightforward to distinguish the "direct decoration" case, but with it it is more difficult.
| # End backward compatiblity. | ||
|
|
||
| if fixture_function and params is None and autouse is False: | ||
| # direct decoration |
There was a problem hiding this comment.
Also curious why id is omitted here (in the line below - GitHub doesn't let me comment on it).
There was a problem hiding this comment.
ids are used for the parametrize which won't be used here due to params is None condition.
There was a problem hiding this comment.
Right. Still seems better to me not to ignore it, so any inconsistencies (like passing ids but not params) will be detected.
| FIXTURE_ARGS_ORDER = ("scope", "params", "autouse", "ids", "name") | ||
|
|
||
|
|
||
| def _parse_fixture_args(callable_or_scope, *args, **kwargs): |
There was a problem hiding this comment.
TBH I'm OK with this implementation; I like that it is general enough that we can reuse it eventually when needed.
Having said that if others prefer the new version I'm OK with that too.
There was a problem hiding this comment.
I don't think we would ever want to reuse it, because it's deprecated and new code should be keyword-only from the start. Any new parameters to pytest.fixture() would be keyword-only too. So this whole code block is "read-only" and I think it's better to have it straightforward and localized (and shorter).
There was a problem hiding this comment.
Yeah I have that feeling like I wanted to reuse this somewhere, but can't really remember it right now. 😁
The dynamic scope feature added in 10bf6aa necessitated some wrangling of arguments in pytest.fixture(). In particular, it deprecated positional arguments in favor of keyword-only arguments, while keeping backward compatibility. The way it did this avoided some code duplication but ended up being quite hard to follow and to annotate with types. Replace it with some straightforward code, which is not very DRY but is simple and easy to remove when the time comes.
bluetech
force-pushed
the
simplify-fixture-compat
branch
from
71e7485 to
03451c3
Compare
|
After looking again I've made some further changes:
|
aklajnert
left a comment
aklajnert
left a comment
There was a problem hiding this comment.
It kinda hurts my eyes seeing so much if statements and makes me want to immediately revert to the previous code (as I'm the original author). But I need to agree, that this version is much easier to read and remove in the future.
| # End backward compatiblity. | ||
|
|
||
| if fixture_function and params is None and autouse is False: | ||
| # direct decoration |
There was a problem hiding this comment.
ids are used for the parametrize which won't be used here due to params is None condition.
😄 Definitely ugly, but it's localized and easier to handle when adding type annotations, so I think the advantages outweigh the ugliness. |
4 participants
The dynamic scope feature added in 10bf6aa necessitated some wrangling of arguments in pytest.fixture(). In particular, it deprecated positional arguments in favor of keyword-only arguments, while keeping backward compatibility.
The way it did this avoided some code duplication but ended up being quite hard to follow and to annotate with types.
Replace it with some straightforward code, which is not very DRY but is simple and easy to remove when the time comes.