Open Kludex opened 11 months ago
@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 python-multipart
?
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 python-multipart
to the (significantly smaller and faster) multipart project, or even vendor it if you prefer. The MIT license would allow shipping multipart.py
as part of starlette.
Disclaimer: I'm the author of multipart
and recently learned that the python-multipart
project causes name conflicts for people because they decided to choose a package name that was already taken. I took that as an opportunity to clean up the project, release a new non-blocking parser that I was working on for a while now, increase test coverage to 100% and today I finally published the 1.0 release. I would obviously love to see starlette depend on multipart
instead of python-multipart
, but I'm probably biased ;)
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.
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 python-multipart
modern (type hints + 100% coverage), so we could merge the parts Starlette use to Starlette itself. But I didn't want to introduce bugs in the way, so the easiest way is just to vendor the package that is already being used.
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: https://github.com/encode/starlette/pull/1514
I'm open to try to make that part of the code customizable in case people want to try custom parsers.
Not sure if that's really necessary. I originally suggested a switch because of the import name conflict between multipart
and python-multipart
(discussed in other issues, off-topic here), but merging python-multipart
into Starlette and no longer installing it as a dependency would also solve the dependency conflict for most users. Once python-multipart
is merged, I do not really see the need for a plug-able parser interface in Starlette.
As a curiosity, there was a PR long ago to replace python-multipart by another parser: https://github.com/encode/starlette/pull/1514
Oh fun, I was first exited to learn about a new parser to benchmark, but it looks like baize
just slightly modified the non-blocking parser from werkzeug (without giving credit).
You could also consider switching from python-multipart to the (significantly smaller and faster) multipart project.
That seems like a really decent idea. (Given the comprehensibility of the code, attention to maintainance & results of benchmarks.)
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'm not sure that's how we tend to operate round these parts? 😊
I'd be interested to see a draft pull request switching MultiPartParser
to using multipart
for it's implementation.
Related partially but if not dropped, can the import be changed?
It's changed already. Bump Starlette.
We should have the multipart parsing on Starlette itself.