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

Replace starlette.datastructures.Headers and other MultiDict-like structures with aio-libs/multidict #640

Closed
jtraub opened this issue Oct 22, 2022 · 5 comments
Assignees
Labels
Enhancement This is a new feature or request

Comments

@jtraub
Copy link
Contributor

jtraub commented Oct 22, 2022

As a part of migration from Starlette we should replace starlette.datastructures.Headers, starlette.datastructures.MutableHeaders and starlette.datastructures.ImmutableMultiDict with another multidict implementation.

Proposed library is multidict

Convenient usage list for mentioned classes can be found in #634

Related issue in starlite-multipart tracker - litestar-org/starlite-multipart#5

@jtraub jtraub added Enhancement This is a new feature or request Triage Required 🏥 This requires triage labels Oct 22, 2022
@jtraub jtraub changed the title Replace starlette.datastructures.Headers and other MutableDict-like structures with aio-libs/multidict Replace starlette.datastructures.Headers and other MultiDict-like structures with aio-libs/multidict Oct 22, 2022
@jtraub jtraub removed the Triage Required 🏥 This requires triage label Oct 22, 2022
@Goldziher Goldziher added Good First Issue This is good for newcomers to take on Help Wanted 🆘 This is good for people to work on starlette-migration and removed Help Wanted 🆘 This is good for people to work on Good First Issue This is good for newcomers to take on labels Oct 22, 2022
@provinzkraut provinzkraut added this to the starlette-migration milestone Oct 22, 2022
@Goldziher
Copy link
Contributor

It occurs to me we can create our own version using https://docs.python.org/3/library/collections.html#collections.ChainMap

This has the advantage if being both fast and a builtin. It's not a multidict, but we can use it as a basis.

Thoughts?

@provinzkraut
Copy link
Member

We could, but I'm not sure if we should. I don't think it's that fast, certainly not faster than the proposed multidict. And we'd have to add most of the implementation, not sure if there's really any benefit to using ChainMap here really (or maybe you have some genius idea on how to do that, that I just can't see? 🙃 If so, please do share!).

@Goldziher
Copy link
Contributor

We could, but I'm not sure if we should. I don't think it's that fast, certainly not faster than the proposed multidict. And we'd have to add most of the implementation, not sure if there's really any benefit to using ChainMap here really (or maybe you have some genius idea on how to do that, that I just can't see? 🙃 If so, please do share!).

Nothing genius I'm afraid. Let's stick with multidicting This

@provinzkraut
Copy link
Member

I had this on my plate already, unless @jtraub wants to take this on I'd take care of it in the following days?

@provinzkraut
Copy link
Member

Implemented in #732

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement This is a new feature or request
Projects
None yet
Development

No branches or pull requests

3 participants