fixtures: make FixtureRequest abstract, add TopRequest subclass#11219
fixtures: make FixtureRequest abstract, add TopRequest subclass#11219bluetech merged 1 commit intopytest-dev:mainfrom
Conversation
| self.names_closure[:] = sorted(closure, key=self.names_closure.index) | ||
|
|
||
|
|
||
| class FixtureRequest: |
There was a problem hiding this comment.
i'd recommend to use the name FixtureRequestBase for a interim with a note that a better name will be found, then we can alias TopRequest to FixtureRequest and/or keep the old name for now
There was a problem hiding this comment.
Not against renaming to FixtureRequestBase, but I like the current implementation where TopRequest and SubRequest implement FixtureRequest -- I would rather not having SubRequest subclass TopRequest like in the previous code.
There was a problem hiding this comment.
Here are my thoughts on this:
The main thing in my mind is, which type do we want users to use for annotating request fixtures in their tests and fixtures?
Currently we only export FixtureRequest, and users use that, and they get pretty much the full public functionality this way. I'm somewhat wary of exposing the "test request" and "fixture request" distinction to the users, I think they wouldn't be able to figure it out that the request name needs different annotations in different situations (and I don't really want them to...).
So I don't think we should rename FixtureRequest to FixtureRequestBase, we don't want the users to care about it being an abstract base class. As mentioned above, my preferred name would have been RequestFixture but I don't think it's worth the pain of deprecation.
As for TopRequest/SubRequest, I would have named them ItemRequestFixture/FixtureRequestFixture but because we don't wan to rename FixtureRequest, they would have to be ItemFixtureRequest/FixtureFixtureRequest which reads funny to me. Also, while SubRequest is private there are still many plugins which use it. So might as well just keep the SubRequest name and then TopRequest to match.
|
|
||
|
|
||
| @final | ||
| class SubRequest(FixtureRequest): |
There was a problem hiding this comment.
we might want to split SubRequest into plain and parametrized while at it
There was a problem hiding this comment.
I'll look into it and report back.
There was a problem hiding this comment.
Looked into it:
In terms of the internal implementation it doesn't add much, mostly allows us to replace a couple of hasattr(request, "param") with isinstance(request, ParametrizedSubRequest).
In terms on the user-facing API/typing, since we're not exposing SubRequest yet it seems premature to consider (see #11219 (comment)). It needs more work I think, assuming we want to expose only a single type to the user.
There was a problem hiding this comment.
mostly allows us to replace a couple of hasattr(request, "param") with isinstance(request, ParametrizedSubRequest).
FTR I find the latter much clearer, but I agree we can wait to decide on this.
|
|
||
|
|
||
| @final | ||
| class SubRequest(FixtureRequest): |
There was a problem hiding this comment.
mostly allows us to replace a couple of hasattr(request, "param") with isinstance(request, ParametrizedSubRequest).
FTR I find the latter much clearer, but I agree we can wait to decide on this.
3 participants
Fix #11218.