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

Proposal: Clean up MultiDict inheritance from ABCs #452

Open
Daverball opened this issue Jun 15, 2023 · 0 comments
Open

Proposal: Clean up MultiDict inheritance from ABCs #452

Daverball opened this issue Jun 15, 2023 · 0 comments

Comments

@Daverball
Copy link

Daverball commented Jun 15, 2023

Currently NestedMultiDict inherits from MultiDict even though it is clearly not a MutableMapping. I understand the convenience of being able to both accept MultiDict and NestedMultiDict with a simple isinstance(x, MultiDict) check, so I propose to add a couple of abstract base classes, namely MultiMapping and MutableMultiMapping which inherit from Mapping and MultiMapping, MutableMapping respectively.

That way you can still accept any immutable MultiDict via MultiMapping but cleanly rule out NestedMultiDict for a function which requires a MutableMultiMapping (or MutableMapping). This is especially relevant for static type checking. At runtime you do emit exceptions with the current implementation, but it will generally end up being deeper in the call-stack than you would prefer. Also the current implementation circumvents hasattr checks.

This would be a breaking change, but I think it is worth it to improve the situation with type checkers. types-WebOb provides good type hints for WebOb but it can't get around this inheritance problem, so NestedMultiMapping can still be passed into functions which it shouldn't be passed into. The type stubs could lie and pretend NestedMultiMapping does not inherit from MultiDict, but that would break the opposite case, where you didn't care about mutability and adding fake types that people need to import inside if TYPE_CHECKING: blocks is bad form.

multidict which is used in aiohttp chose this exact approach (they have abstract MultiMapping and MutableMultiMapping classes), so you could also just add it as a new dependency and reuse their implementation and extend it with NoVars and NestedMultiDict (which should inherit from MultiMapping) to keep things backwards compatible. The advantage of that approach would be that multidict has a C extension, so it should be significantly faster than this pure-python implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant