TypeConversionDict.pop with type

[MarcinKonowalczyk]

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

[MarcinKonowalczyk]

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]

@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]

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]

Above, we raise the error if the default is missing. Seems like it should be raised here too, not silently succeed.

[davidism]

I pushed an updated version with this change. Leaving this open for discussion. Didn't fix the tests yet.

[davidism]

After thinking a bit more, I still think raising the error makes sense. dict.pop(key) will raise an error if the key doesn't exist, dict.pop(key, None) can be used to silence that error. Extending that to "key exists but conversion fails" makes sense.

[davidism]

This isn't valid unless the implementation of MultiDict.pop is changed to have a type parameter.

Similarly, poplist should probably have a type parameter.

[davidism]

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]

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]

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.