-
Notifications
You must be signed in to change notification settings - Fork 148
fix: warn_deprecated now respect warnings.filter ignores #1476
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
base: master
Are you sure you want to change the base?
Conversation
…arnings - checks by checking module name and action
ignore deprecation w…|
I'm not sure how the pyright check is failing |
|
@onerandomusername pyright tests are borked |
Enegg
left a comment
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.
I'm not sure we really need this hack to be in the lib in the first place, but since this is legacy behavior I'm not one to decide that
disnake/utils.py
Outdated
| if send_warning: | ||
| # this still allows force bypassing of filters if the default DeprecationWarning ignore is set. | ||
| try: | ||
| warnings.simplefilter(action="always", category=DeprecationWarning) | ||
| warnings.warn(*args, stacklevel=stacklevel + 1, category=DeprecationWarning, **kwargs) | ||
| finally: | ||
| # NOTE: Is this assertion even necessary? warnings.filters is always at minimum an empty list? | ||
| assert isinstance(warnings.filters, list) | ||
| warnings.filters[:] = old_filters |
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.
| if send_warning: | |
| # this still allows force bypassing of filters if the default DeprecationWarning ignore is set. | |
| try: | |
| warnings.simplefilter(action="always", category=DeprecationWarning) | |
| warnings.warn(*args, stacklevel=stacklevel + 1, category=DeprecationWarning, **kwargs) | |
| finally: | |
| # NOTE: Is this assertion even necessary? warnings.filters is always at minimum an empty list? | |
| assert isinstance(warnings.filters, list) | |
| warnings.filters[:] = old_filters | |
| # allow force bypassing of filters if the default DeprecationWarning ignore is set. | |
| if not send_warning: | |
| return | |
| try: | |
| warnings.simplefilter(action="always", category=DeprecationWarning) | |
| warnings.warn(*args, stacklevel=stacklevel + 1, category=DeprecationWarning, **kwargs) | |
| finally: | |
| assert isinstance(warnings.filters, list) | |
| warnings.filters[:] = old_filters |
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.
agree on the guard clause instead and committed your suggestion. Just thought, after committing we could instead return within the for loop of old_filters instead of assigning send_warning a value and breaking from the loop. Personally, I don't like that as it's not as explicit
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 bool variable can be avoided by using a for ... else clause, while still keeping it flat.
for ... in ...:
if condition:
break
else:
return
# here goes stuff that should run when condition is trueThere 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.
Instead of the early break, i've settled for doing an early return here instead
Co-authored-by: Eneg <[email protected]> Signed-off-by: Sam <[email protected]>
Co-authored-by: Eneg <[email protected]> Signed-off-by: Sam <[email protected]>
- updated tests to be more explanatory if they fail assertions
- also removed the cheeky change of old_filters >= and replaced with >
Summary
Closes #1475
warn_deprecated now respect if a user wants to ignore deprecation warnings by checking if the module name has
disnakein it.test_deprecated_warntests have been added.Expected use will be:
Using simplefilter or not passing a module value will result in the existing behaviour now which forces through the warning, as such this change will not impact current users unless they use the above example, or similar.
warning filters are generally not that large, so there is expected to be a very negligible impact to performance.
Known potential issues with this approach
ignoreaction value for the warnings.disnakein it will be picked up to be ignored if the action isignoreChecklist
uv run nox -s lintuv run nox -s pyright