Pylons / Pylons/webob

Proposal: Clean up `MultiDict` inheritance from ABCs

Open
#452 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

sprintable
Dominant language
Python
Stars
443
Forks
206
PR merge metrics
No merged PRs in 30d

Description

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reviewing the existing MultiDict and NestedMultiDict classes and compare the abstract MultiMapping and MutableMultiMapping approach used by multidict. Determine whether WebOb should implement the proposal or reuse multidict, then verify that mutability checks, isinstance behavior, hasattr checks, and static typing remain correct for the chosen API.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend-api-design
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.