search

pallets / werkzeug

The comprehensive WSGI web application library.
https://werkzeug.palletsprojects.com
BSD 3-Clause "New" or "Revised" License
6.65k stars 1.73k forks source link

TypeConversionDict.pop with type #2888

Closed MarcinKonowalczyk closed 1 week ago

MarcinKonowalczyk commented 6 months ago

Proposed implementation of pop with type parameter on TypeConversionDict. Address #2883.

MarcinKonowalczyk commented 6 months ago

Typing is slightly messy, but I think correct. mypy certainly seems to be happy with it. I've had to battle a bit with ImmutableTypeConversionDict typing, but won in the end.

MarcinKonowalczyk commented 4 months ago

@davidism anything else which needs to happen here? (all good if just waiting / no-one gotten to it. just wanna make sure i'm not sitting idle on something i'm supposed to be doing here)

MarcinKonowalczyk commented 4 months ago

Since I often use this in a non-werkzeug context, I've extracted the TypeConversionDict into a single-file micro-package: https://github.com/MarcinKonowalczyk/type_conversion_dict.

I thought it relevant to post here since i've done quite a bit of test + typing work there. The get and pop signatures are compatible with dict and werzeug.datastructures.TypeConversionDict.

In that implementation i've also added a required kwarg which I find very useful but i don't propose to add it here ( at least not in this PR/Issue. Obv feel free to add it too if you like it :)) )

davidism commented 1 week ago

Hmm, I'm starting to wonder if this is worth it. There's also popitem, and for MultiDict there's poplist and popitemlist, and there's also OrderedMultiDict.

Nothing in Werkzeug uses MultiDict destructively, it's either immutable, or additive-only during testing. Long term, I would like to simplify our datastructures implementation, as there is a lot of stuff we don't actually use in Werkzeug that adds complexity.

It seems like a general MultiDict/immutable implementation would be more appropriate as a separate library. Most users probably aren't reaching for an entire WSGI library just for MultiDict. That could also do away with Werkzeug-specific stuff like BadRequestKeyError.

davidism commented 1 week ago

Pushed implementations of MultiDict pop, popitem, poplist, and popitemlist, but no tests or type annotations. I mostly did it as an exercise, I'm still not convinced this should go in.

davidism commented 1 week ago

After discussing it with other maintainers, we don't want to accept the additional complexity right now, when Werkzeug itself won't use it. Thanks for working on this.