-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
ENH: Add all warnings check to the assert_produces_warnings, and separate messages for each warning. #57222
ENH: Add all warnings check to the assert_produces_warnings, and separate messages for each warning. #57222
Conversation
…parate messages for each warning.
@rhshadrach Could you review this? Or direct me to someone who can? It's my first contribution here, so I don't know who to ask. |
I should be able to get to this tomorrow. |
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 for the PR!
pandas/_testing/_warnings.py
Outdated
type must get encountered. Otherwise, even one expected warning | ||
results in success. | ||
|
||
.. versionadded:: 3.0.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.
This function isn't in the documentation; this line can be removed.
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.
Done
pandas/_testing/_warnings.py
Outdated
match=match, | ||
check_stacklevel=check_stacklevel, | ||
) | ||
if type(expected_warning) == tuple and must_find_all_warnings: |
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.
Should use isinstance
here I think, hopefully making the cast below unnecessary.
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.
Thank you, that helped to remove most of the casts.
b4d9fc0
to
9093962
Compare
3523b7b
to
c6d6983
Compare
…new API of `assert_produces_warning`
c6d6983
to
2242796
Compare
# Conflicts: # pandas/tests/copy_view/test_chained_assignment_deprecation.py # pandas/tests/frame/methods/test_interpolate.py # pandas/tests/indexing/test_chaining_and_caching.py
@rhshadrach Could you take another look at this, when you have time? I addressed all change requests and resolved all merge conflicts. The docstring checks seem to still be failing, but it looks unrelated to the changes introduced in this PR. |
# Conflicts: # pandas/tests/io/parser/common/test_inf.py
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 looking nice!
pandas/_testing/_warnings.py
Outdated
check_stacklevel=check_stacklevel, | ||
) | ||
else: | ||
expected_warning = cast(type[Warning], expected_warning) |
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 can still be a tuple, no?
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, you're right. I believe this was originally done, to suppress _assert_caught_expected_warning
not accepting tuples. I changed that and fixed some issues that arose because of that now.
pandas/_testing/_warnings.py
Outdated
match = ( | ||
match | ||
if isinstance(match, tuple) | ||
else tuple(match for i in range(len(expected_warning))) |
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.
nit: Use (match,) * len(expected_warning)
instead.
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.
Done
@rhshadrach I just applied a patch that fixed the python 3.9 unit test not passing. With this, this should be ready for review. |
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.
lgtm, @mroeschke - wouldn't mind a 2nd eye on this if you get the chance.
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.
Could you add some tests to pandas/tests/util/test_assert_produces_warning.py
? e.g. multiple warnings and matches, multiple warnings and maches don't match, etc
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
@Jorewin - are you interested in finishing this up? If not, I can add some tests to get it over the finish line. |
@rhshadrach If you want to, then go ahead. I just didn't have much time lately. |
@mroeschke - tests added for various combinations. Let me know if you see any others to add. |
Thanks @Jorewin and @rhshadrach |
…rate messages for each warning. (pandas-dev#57222) * ENH: Add all warnings check to the `assert_produces_warnings`, and separate messages for each warning. * Fix typing errors * Fix typing errors * Remove unnecessary documentation * Change `assert_produces_warning behavior` to check for all warnings by default * Refactor typing * Fix tests expecting a Warning that is not raised * Adjust `raises_chained_assignment_error` and its dependencies to the new API of `assert_produces_warning` * Fix `_assert_caught_expected_warning` typing not including tuple of warnings * fixup! Refactor typing * fixup! Fix `_assert_caught_expected_warning` typing not including tuple of warnings * fixup! Fix `_assert_caught_expected_warning` typing not including tuple of warnings * Add tests --------- Co-authored-by: Richard Shadrach <[email protected]>
The function in question gets used in multiple places, that's why I felt it would be safer to preserve current behavior and API, and add an additional, optional parameter, which alters the behavior, enabling checking if all passed warning types get detected.
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.