Add exception to value alias on ExceptionInfo #5541
Conversation
|
imho this should also issue a warning so users can fix their code up |
|
@RonnyPfannschmidt totally agree, what warning should it be? Deprecation warning feels fundamentally wrong here |
There was a problem hiding this comment.
Thanks @graingert!
See my suggestion about which warning and message to use.
Please target features and add a CHANGELOG entry. 👍
graingert
force-pushed
the
patch-2
branch
2 times, most recently
from
cfa1cf3 to
127de96
Compare
For easier compatibility with unittest pytest-dev/unittest2pytest#36 Co-Authored-By: Bruno Oliveira <[email protected]>
|
@nicoddemus this should also apply to pytest.warns |
Co-Authored-By: Bruno Oliveira <[email protected]>
|
@nicoddemus eg aliases for all these: https://github.com/python/cpython/blob/3.7/Lib/unittest/case.py#L248-L261 |
Not sure if all of those, but |
|
@graingert still want to add the aliases for |
|
looking at it now |
|
@nicoddemus it's actually a bit of a pain as it would involve chainging how WarningsChecker works, eg it doesn't actually store the first matching warning anywhere |
|
@nicoddemus looks a bit like graingert@4a2ced9 |
|
I see. Well I think that's fine if indeed we want to add those aliases. 👍 |
There was a problem hiding this comment.
Thanks @graingert for the follow up! Please take a look at my comments when you have the chance. 👍
| @@ -186,28 +186,33 @@ def __exit__(self, *exc_info): | |||
| __tracebackhide__ = True | |||
|
|
|||
| # only check if we're not currently handling an exception | |||
There was a problem hiding this comment.
We need a test for the warning attribute to avoid regressions.
| @@ -0,0 +1 @@ | |||
| Add ``.exception`` attribute as an alias for ``.value`` to facilitate porting tests written using unittest. | |||
There was a problem hiding this comment.
This should be updated to mention the warning attribute of pytest.warns
There was a problem hiding this comment.
Oh this should also be 5541.improvement.rst now.
| if issubclass(r.category, self.expected_warning) and ( | ||
| self.match_expr is None or re.search(self.match_expr, str(r.message)) | ||
| ): | ||
| self.warning = r.message |
There was a problem hiding this comment.
These attributes should be initialized with None in __init__
There was a problem hiding this comment.
Also, we should add a quick example to the docs to warnings.rst. 👍
|
@graingert gentle ping |
|
Looks like this needs a rebase etc |
Indeed, also please see my comments. 👍 |
| @property | ||
| def exception(self): | ||
| """ | ||
| an alias to '.value' to facilitate porting porting tests written using |
There was a problem hiding this comment.
| an alias to '.value' to facilitate porting porting tests written using | |
| an alias to '.value' to facilitate porting tests written using |
This comment has been minimized.
This comment has been minimized.
|
TBH I'm not sure this is worthwhile doing anymore... perhaps it is better to improve unittest2pytest to deal with this automatically, and mention it more prominently in the docs? |
(I've changed the base on my own now). |
|
I agree with @nicoddemus; let's close this. |
For easier compatibility with unittest pytest-dev/unittest2pytest#36