Skip to content

feat: Respect user warning filters for deprecation messages #2853

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

Merged
merged 2 commits into from
Aug 6, 2025

Conversation

Lumabots
Copy link
Contributor

@Lumabots Lumabots commented Aug 3, 2025

Summary

This PR removes the calls to warnings.simplefilter("always", DeprecationWarning) and warnings.simplefilter("default", DeprecationWarning) in the deprecation helper.

Currently, the implementation forces DeprecationWarning visibility by temporarily setting the filter to "always", and then resets it to "default". This overrides user-defined warning filters, which is not ideal.

With this change, the function simply calls warnings.warn, leaving all filtering behavior up to the user or their framework (e.g. pytest, PYTHONWARNINGS, warnings.filterwarnings).

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • This PR is not a code change (e.g. documentation, README, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

@Lumabots
Copy link
Contributor Author

Lumabots commented Aug 3, 2025

i dont think this deserve a changelog

@Lumabots
Copy link
Contributor Author

Lumabots commented Aug 3, 2025

Screenshot 2025-08-04 at 01 48 08

@Dorukyum
Copy link
Member

Dorukyum commented Aug 5, 2025

Hold on, does the default behavior make the deprecation warnings visible?

@Lulalaby
Copy link
Member

Lulalaby commented Aug 5, 2025

According to a quick research, no. Users would've to opt-in

@Paillat-dev
Copy link
Member

Would it be possible for pycord to make it opt out instead without forcing them

@Lulalaby
Copy link
Member

Lulalaby commented Aug 6, 2025

I think that's what warnings.simplefilter("default", DeprecationWarning) is for

@Lumabots
Copy link
Contributor Author

Lumabots commented Aug 6, 2025

I didn't test it yet, I'll test this tonight
But for me the edit back to default will reset the user settings so shouldn't be use at all.
And same so for the always which doesn't make any sense to me

@Lumabots
Copy link
Contributor Author

Lumabots commented Aug 6, 2025

after testing, its working great without those 2 lines, we can easily ignore those warning globally or within a with ..

@Lulalaby
Copy link
Member

Lulalaby commented Aug 6, 2025

So the default configuration used by python does work?

@Lumabots
Copy link
Contributor Author

Lumabots commented Aug 6, 2025

So the default configuration used by python does work?

The default one does raise the warning, so we don't need to add any filter.

Copy link
Member

@Lulalaby Lulalaby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me then

@Paillat-dev
Copy link
Member

Do you have some docs / other ref about the default behaviour ?

@Lulalaby Lulalaby merged commit b62052a into Pycord-Development:master Aug 6, 2025
28 checks passed
@Lumabots
Copy link
Contributor Author

Lumabots commented Aug 6, 2025

Do you have some docs / other ref about the default behaviour ?

https://docs.python.org/3/library/warnings.html

but its mainly about the filters, there is no need to add a filter here since we are not filtering anything but emitting a warn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants