-
-
Notifications
You must be signed in to change notification settings - Fork 952
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
Drop python-multipart
dependency
#2390
Comments
@Kludex, I'd like to take on this task, please assign me. I've understood the parser classes and written test cases for them. I've also started implementing the parser functionality to phase out the dependency on python-multipart. I will try to complete this in some days and will submit a PR then. Could you provide any specific things or best practices I should follow during the implementation? Additionally, would it be preferable to push the entire change at once or make smaller, incremental PRs? Thanks! |
what's wrong with |
Nothing. |
I think the point is that even if nothing is wrong with it Starlette should still absorb that functionality since it's quite core to what Starlette does. |
I'd rather extract functions like this from existing projects, for greater modularization and to facilitate re-use, but you be you. |
You could also consider switching from Disclaimer: I'm the author of Edit: I learned after writing this comment that python-multipart and starlette are developed by the same people, so switching to a different implementation is probably not an option for non-technical reasons. |
I mean, the idea here was to first make I'm open to try to make that part of the code customizable in case people want to try custom parsers. |
As a curiosity, there was a PR long ago to replace python-multipart by another parser: #1514 |
Not sure if that's really necessary. I originally suggested a switch because of the import name conflict between
Oh fun, I was first exited to learn about a new parser to benchmark, but it looks like |
That seems like a really decent idea.
I'm not sure that's how we tend to operate round these parts? 😊 I'd be interested to see a draft pull request switching |
It's changed already. Bump Starlette. |
We should have the multipart parsing on Starlette itself.
Important
The text was updated successfully, but these errors were encountered: