-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[WIP] typing: fix _pytest._code.source.getfslineno: returns str; assignment #6590
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
src/_pytest/_code/source.py
Outdated
@@ -305,11 +310,7 @@ def getfslineno(obj) -> Tuple[Optional[Union["Literal['']", py.path.local]], int | |||
_, lineno = findsource(obj) | |||
except IOError: | |||
pass |
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
We've discussed the fspath = fn and py.path.local(fn) or None
line above recently (#6527 (comment)).
I think it might actually never happen (and will try to write a test for it), but in general I think we should remove the Optional
from the return type, which would simpify typing internally already.
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 is relevant for
Lines 310 to 317 in 0e777a6
def getfslineno(obj) -> Tuple[Union[str, py.path.local], int]: | |
# xxx let decorators etc specify a sane ordering | |
obj = get_real_func(obj) | |
if hasattr(obj, "place_as"): | |
obj = obj.place_as | |
fslineno = _pytest._code.getfslineno(obj) | |
assert isinstance(fslineno[1], int), obj | |
return fslineno |
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 depends on whether fn = inspect.getsourcefile(obj) or inspect.getfile(obj)
can end up being falsey. I didn't check.
Fixes: > src/_pytest/_code/source.py:309: error: Incompatible types in > assignment (expression has type "Union[local, str]", variable has type > "Optional[local]") [assignment]
2c3702e
to
0e777a6
Compare
src/_pytest/_code/source.py
Outdated
@@ -282,7 +283,7 @@ def compile_( # noqa: F811 | |||
return s.compile(filename, mode, flags, _genframe=_genframe) | |||
|
|||
|
|||
def getfslineno(obj) -> Tuple[Optional[Union["Literal['']", py.path.local]], int]: | |||
def getfslineno(obj: Any) -> Tuple[Optional[Union[str, py.path.local]], int]: |
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
Not sure about obj: Any
here. I've tried Callable[[Any], None]
inbetween, but that would e.g. not cover classes etc.
I think it makes sense to allow Any here explicitly, given that TypeErrors are handled etc anyway.
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, looks like this really is supposed to handle everything. I'd make it object
though, instead of Any
. object
means the code can assume nothing on the value, while Any
means the code can assume anything.
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.
Then it complains with the inspect.get{source,}file
calls though, which expects "Union[Module, Type[Any], MethodType, FunctionType, TracebackType, FrameType, CodeType, Callable[..., Any]]"
.
Should we use this then maybe also, or # type: ignore
it?
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.
Catching TypeError
is basically a runtime type: ignore
. So seems justifiable in this case.
(Of course, eventually it will be good to get rid of this except TypeError
, which indicates some type confusion)
src/_pytest/_code/source.py
Outdated
@@ -282,7 +283,7 @@ def compile_( # noqa: F811 | |||
return s.compile(filename, mode, flags, _genframe=_genframe) | |||
|
|||
|
|||
def getfslineno(obj) -> Tuple[Optional[Union["Literal['']", py.path.local]], int]: | |||
def getfslineno(obj: Any) -> Tuple[Optional[Union[str, py.path.local]], int]: |
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, looks like this really is supposed to handle everything. I'd make it object
though, instead of Any
. object
means the code can assume nothing on the value, while Any
means the code can assume anything.
src/_pytest/_code/source.py
Outdated
@@ -305,11 +310,7 @@ def getfslineno(obj) -> Tuple[Optional[Union["Literal['']", py.path.local]], int | |||
_, lineno = findsource(obj) | |||
except IOError: | |||
pass |
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 depends on whether fn = inspect.getsourcefile(obj) or inspect.getfile(obj)
can end up being falsey. I didn't check.
It should be in `test_code` when testing `_pytest._code.getfslineno`, not to be confused with `_pytest._code.source.getfslineno`. Adds an extra assert (via pytest-dev#6590).
It should be in `test_code` when testing `_pytest._code.getfslineno`, not to be confused with `_pytest._code.source.getfslineno`. Adds an extra assert (via pytest-dev#6590).
Moved into #6656. |
Fixes: