-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix type errors after adding types to the py
dependency
#6511
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
@@ -426,7 +426,8 @@ def module(self): | |||
@scopeproperty() | |||
def fspath(self) -> py.path.local: | |||
""" the file system path of the test module which collected this test. """ | |||
return self._pyfuncitem.fspath | |||
# TODO: Remove ignore once _pyfuncitem is properly typed. | |||
return self._pyfuncitem.fspath # type: ignore |
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.
Use the error code here? (see --show-error-codes
, could be added to the config probably) (with flake-ignore then)
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.
We should probably do them all at once, maybe on the next mypy bump.
@@ -462,6 +462,7 @@ def reportinfo(self) -> Tuple[Union[py.path.local, str], Optional[int], str]: | |||
@cached_property | |||
def location(self) -> Tuple[str, Optional[int], str]: | |||
location = self.reportinfo() | |||
assert isinstance(location[0], py.path.local), location[0] |
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.
You've removed the change from reportinfo, right? (from the initial Git tree)
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.
Yes.
class MyFunction(pytest.Function): | ||
def reportinfo(self): | ||
return "ABCDE", 42, "custom" | ||
return py.path.local("foo"), 42, "custom" |
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.
That's kind of an API change - I've noticed this myself that the tests seem to be the only things using str
.
Ok with me in general, just wanted to mention this, i.e. some plugins might return strings there (but likely fail then afterwards, depending on where it is used I guess?)
Maybe this could/should have a test?
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.
The _bestrelpathcache
backing this only works with py.path.local
, that it returned something for str
was accidental and incorrect. So I think the assert just codifies the current behavior.
Only this test trigerred the assert, and it's just a mock, so I think it should be fine.
This is in preparation for adding type stubs to
py
. See pytest-dev/py#232.These changes make mypy clean both before and after.