-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[WIP] typing improvements #6449
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
Conversation
|
||
def _prepareconfig(args=None, plugins=None): | ||
def _prepareconfig( | ||
args: Optional[Union[List, Tuple]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this can also be a py.path.local
. Even though it's not a real type yet, if you recall we decided to add it anyway so once typing is added to py.path
it will start working.
|
||
def get_config(args=None, plugins=None): | ||
def get_config( | ||
args: Optional[Union[List, Tuple]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List
and Tuple
without a []
is equivalent to List[Any]
and Tuple[Any]
. Are there narrower types that can be used?
If yes, it's OK to leave them out for now, leaving them out indicates a TODO.
If no, I prefer to put explicit List[Any]
, this it's clear it can really be anything and is not just omitted/TODO.
|
||
def __init__(self, pluginmanager, *, invocation_params=None) -> None: | ||
def __init__( | ||
self, pluginmanager: "PytestPluginManager", *, invocation_params=None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe invocation_params: Optional[Config.InvocationParams]
?
(OK if you left it out intentionally).
|
||
|
||
def get_parametrized_fixture_keys(item, scopenum): | ||
def get_parametrized_fixture_keys( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on how much effort you're willing to expand on this function, it looks like this can be something like this:
@overload
def get_parametrized_fixture_keys(item: "nodes.Item", scopenum: Literal[0]) -> Iterator[Tuple[str, int]]: ...
@overload
def get_parametrized_fixture_keys(item: "nodes.Item", scopenum: Literal[1, 2]) -> Iterator[Tuple[str, int, py.path.local]]: ...
@overload
def get_parametrized_fixture_keys(item: "nodes.Item", scopenum: Literal[4]) -> Iterator[Tuple[str, int, py.path.local, "Class"]]: ...
Whether it needs scopenum: int
fallback depends on the callers.
from typing import Type | ||
|
||
_PluggyPlugin = object | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bluetech since this might be used elsewhere - where should be put this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long is we keep it internal for now (I mean, not intended to be exposed in the "public" typing in the future), I guess putting it where PytestPluginManager
is defined makes most sense?
A short comment here would be helpful, something like
A type to represent plugin objects. Plugins can be any namespace, so we can't narrow it down much, but we use an alias to make the intent clear. Ideally this type would be provided by pluggy itself.
BTW, I wouldn't put it under TYPE_CHECKING
, this way we can use it without the annoying quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blueyed Are you working on this PR? |
No. Some things have been taken out of it already, and I plan to revisit it to see if anything is left. |
Closing for now. |
@bluetech |
No description provided.