typing around Node.location, reportinfo, repr_excinfo etc #6129
Conversation
| truncate_locals: bool = True, | ||
| chain: bool = True, | ||
| ): | ||
| ) -> Union["ReprExceptionInfo", "ExceptionChainRepr"]: |
There was a problem hiding this comment.
Functions returning a Union are not great, because the caller must disambiguate which one they got.
In this case, both types share a common base class ExceptionRepr. If all the caller cares about is getting an ExceptionRepr from this function, I suggest using that instead. This will also be more future proof.
If however the distinction is important (not in this case AFAIU), I think it will be better to either use two overloads on style, Literal["long"] which only returns ExceptionChainRepr and Literal["native"] which only returns ReprExceptionInfo. Or just split the function to two.
There was a problem hiding this comment.
Thanks for the input.
I've tried to be as strict / verbose as possible, given the code around all this.
Will consider using ExceptionRepr then.
There was a problem hiding this comment.
Not sure if you decided to keep it this way.
Other than this, LGTM.
There was a problem hiding this comment.
Yeah, better have it (re)strict(ed) for now.
src/_pytest/nodes.py
Outdated
| # Type ignored: see comment where fail.Exception is defined. | ||
| if excinfo.errisinstance(fail.Exception): # type: ignore | ||
| def _repr_failure_py( | ||
| self, excinfo: ExceptionInfo[Union[Failed, "FixtureLookupError"]], style=None |
There was a problem hiding this comment.
Are we sure only Failed and FixtureLookupError are possible here?
There was a problem hiding this comment.
That are the ones I've found - can be adjusted if needed?
There was a problem hiding this comment.
It depends on what the intention of the function is - to accept only these two types, or to accept any ExceptionInfo[E].
There was a problem hiding this comment.
My intention is to being made aware if other types are used.
This should aid with refactoring / cleanup - my main motivation to add types here.
Ref: pytest-dev#6129 Conflicts: .travis.yml src/_pytest/_code/code.py
src/_pytest/reports.py
Outdated
| self.outcome = outcome | ||
| self.longrepr = longrepr | ||
| self.result = result or [] | ||
| self.result = result or [] # type: List |
There was a problem hiding this comment.
Do you what type of list it is?
There was a problem hiding this comment.
Via CallInfo.from_call usually: https://github.com/blueyed/pytest/blob/1b3c0e6d5c078dadd11dfb2a109dc330742ac75c/src/_pytest/runner.py#L210-L236
There was a problem hiding this comment.
So can this be List[CallInfo]?
(btw, it's perfectly fine if you left it out because it's too much work to check, I'm just commenting in case you forgot to fill it in).
There was a problem hiding this comment.
No, CallInfo sets the result to the return value from the functions.
Can be List[Node] then, will change it.
2 participants
No description provided.