Skip to content
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

fix: reduce visibility of warning for next release #174

Merged
merged 1 commit into from
Oct 27, 2024

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Oct 26, 2024

This reduces the visibility of the warning; you have to have -Wdefault or -Werror to see it. It can be increased to a FutureWarning in a subsequent release, but this gives packages more time to adapt.

We could remove the warning entirely, but the warning was what I was using to test that it worked, so I think this is better. Happy to change if you'd prefer no warning at all.

@Kludex
Copy link
Owner

Kludex commented Oct 27, 2024

Thanks 🙏

@Kludex Kludex merged commit 5578583 into Kludex:master Oct 27, 2024
6 checks passed
@risicle
Copy link

risicle commented Nov 2, 2024

Did you consider omitting the warning entirely for a few months to give people a chance to switch without the warning e.g. breaking their CI? At the moment there's no way for a depending project to both use the new import-name itself, yet not constrain the python-multipart version to one that will potentially break other projects' (arguably overly-fussy) CI.

This would ease the transition and avoid other projects just pinning to old versions (where they have to use the old import-name).

@henryiii
Copy link
Contributor Author

henryiii commented Nov 2, 2024

This warning is very hard to see, you have to have all warnings enabled. Which I think is exactly what you’d enable all warnings for, you want to be aware of incoming changes early. A normal user won’t see them.

You can suppress the warning or try to import the new name first, the fallback on the old one.

But yes, did consider it, but we’d have lost the ability to test it was working, as the test enables and looks for the warning.

@defnull
Copy link
Contributor

defnull commented Nov 2, 2024

Hm. PendingDeprecationWarning is already a very weak signal and ignored by default. It should not affect production at all, only show up in tests (which is intentional). CI should warn, but not break on those warnings.

Is this a theoretical issue, or do you have a specific example for a project that depends on python-multipart, uses the old import name, and rejected or ignored an issue or PR to fix that?

@henryiii
Copy link
Contributor Author

henryiii commented Nov 2, 2024

If you've turned all warnings to errors in your CI, I'd highly recommend:

try:
    import python_multipart as multipart
except ModuleNotFoundError:
    import multipart

Otherwise, you won't get the benefit of what this is supposed to fix (allowing python_multipart and multipart to be installed at the same time).

@risicle
Copy link

risicle commented Nov 2, 2024

It causes starlette's tests to fail, so I presume it also breaks some others'.

@defnull
Copy link
Contributor

defnull commented Nov 2, 2024

The most recent release of starlette should not trigger the warning. It tries import python_multipart first, then falls back to import multipart in case an older version of python-multipart is installed.

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