-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Type annotate main.py and some parts related to collection #6556
Conversation
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.
Left comments on where I needed to make code changes, for greater scrutiny.
m = self.matchnodes(col, names) | ||
# If __init__.py was the only file requested, then the matched node will be | ||
# the corresponding Package, and the first yielded item will be the __init__ | ||
# Module itself, so just use that. If this special case isn't taken, then all | ||
# the files in the package will be yielded. | ||
if argpath.basename == "__init__.py": | ||
assert isinstance(m[0], nodes.Collector) |
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.
Needed to add an assert here (can also be a nodes.Item
), but seems safe given it should be a Package
and .collect()
is called just below.
try: | ||
yield next(m[0].collect()) | ||
yield next(iter(m[0].collect())) |
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.
Needed to add iter()
here because I made the official type of collect()
be Iterable
and not Iterator
(which isn't the case for all implementations). Calling iter()
on an iterator should just be idempotent, so I think it's safe.
@@ -651,30 +715,32 @@ def _tryconvertpyarg(self, x): | |||
# ValueError: not a module name | |||
except (AttributeError, ImportError, ValueError): | |||
return x | |||
if spec is None or spec.origin in {None, "namespace"}: | |||
if spec is None or spec.origin is None or spec.origin == "namespace": |
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.
Had to split it because mypy doesn't handle in {None}
for type narrowing.
if not safe_getattr(self.obj, "__test__", True): | ||
return [] | ||
if hasinit(self.obj): | ||
assert self.parent is not 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.
This assert and below should be safe since dereferenced just below.
src/_pytest/main.py
Outdated
@@ -391,22 +412,30 @@ class Session(nodes.FSCollector): | |||
_setupstate = None # type: SetupState | |||
# Set on the session by fixtures.pytest_sessionstart. | |||
_fixturemanager = None # type: FixtureManager | |||
exitstatus = None # type: Union[int, ExitCode] |
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.
Didn't find any place which does hasattr('exitstatus')
or such, so I think this is safe.
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.
It's still kind of an API change.
But I also think it is good to have it being set always.
# we are inside a make_report hook so | ||
# we cannot directly pass through the exception | ||
self._notfound.append((report_arg, sys.exc_info()[1])) | ||
exc = cast(NoMatch, sys.exc_info()[1]) |
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.
I don't really follow the comment above. I think this cast is correct...
src/_pytest/main.py
Outdated
@@ -391,22 +412,30 @@ class Session(nodes.FSCollector): | |||
_setupstate = None # type: SetupState | |||
# Set on the session by fixtures.pytest_sessionstart. | |||
_fixturemanager = None # type: FixtureManager | |||
exitstatus = None # type: Union[int, ExitCode] |
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.
It's still kind of an API change.
But I also think it is good to have it being set always.
This comment has been minimized.
This comment has been minimized.
5c1256f
to
49f830d
Compare
Addressed (some) comments and rebased on features. This is still blocked. |
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.
Thanks @bluetech! Comments below, but before all of that thanks: we really appreciate your work on this 😁
Taken out of pytest-dev#6556.
Taken out of pytest-dev#6556.
Taken out of pytest-dev#6556.
49f830d
to
eb648ab
Compare
Taken out of pytest-dev#6556.
Taken out of pytest-dev#6556.
Pulled out of pytest-dev#6556.
Pulled out of pytest-dev#6556.
eb648ab
to
8430b21
Compare
This comment has been minimized.
This comment has been minimized.
f1e712f
to
bde54d1
Compare
Rebased on |
bde54d1
to
abdd513
Compare
Maintaining all of these open PRs is too annoying and spammy. I'm going to close them and maintain just a single |
Depends on #6547 and #6555 (and includes them until merged).This adds type annotations to main.py. To do this, I needed to feel in some blanks in other parts, mostly related to collection.
It took some effort to untangle but I think I got it mostly right...