Skip to content

Fix CIMultiDictProxy inheritance docs and istr typo#1298

Open
veeceey wants to merge 2 commits intoaio-libs:masterfrom
veeceey:fix/docs-cimultidictproxy-and-istr
Open

Fix CIMultiDictProxy inheritance docs and istr typo#1298
veeceey wants to merge 2 commits intoaio-libs:masterfrom
veeceey:fix/docs-cimultidictproxy-and-istr

Conversation

@veeceey
Copy link
Contributor

@veeceey veeceey commented Feb 10, 2026

Summary

  • Fix CIMultiDictProxy documentation: states it inherits from MultiDict but it actually inherits from MultiDictProxy
  • Fix missing word in istr section: "explicitly hand" -> "explicitly by hand"

Test plan

  • Documentation-only changes

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 10, 2026

Merging this PR will not alter performance

✅ 245 untouched benchmarks


Comparing veeceey:fix/docs-cimultidictproxy-and-istr (e28eda7) with master (dbebab0)

Open in CodSpeed

@webknjaz
Copy link
Member

We need context on why it was written like that originally. Git/GitHub archeology, that sort of stuff.

Best do it by hand, Claude isn't good at extracting implicit common knowledge. Though, you might be able to make it load some context via GH CLI, not sure.

The grammar fix looks fine, I'd accept it already if it was atomic and not coupled with a change that lacks justification.

cc @bdraco @Dreamsorcerer need a second opinion on what that inheritance mention might've meant to imply.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR corrects inaccuracies in the multidict documentation so it matches the actual class hierarchy and fixes a small typo in the istr section.

Changes:

  • Fix CIMultiDictProxy docs to state it inherits from MultiDictProxy (matching the implementation).
  • Fix typo in istr documentation: “explicitly hand” → “explicitly by hand”.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Feb 10, 2026

cc @bdraco @Dreamsorcerer need a second opinion on what that inheritance mention might've meant to imply.

It's just documenting the base class. The change is correct, though I'd have thought Sphinx has a built-in way to document parent classes that would be better to use (than free-form text)?

@webknjaz
Copy link
Member

though I'd have thought Sphinx has a built-in way to document parent classes that would be better to use (than free-form text)?

Yes. .. autoclass:: et al. I personally like the apidoc integrations but historically Andrew wanted to follow the CPython traditions (which I disapprove of).

@webknjaz
Copy link
Member

It's just documenting the base class.

It just seems like it might've been talking about the initializer argument being of type rather than the class itself. I'd be interested to see the context in which it first appeared.

@veeceey
Copy link
Contributor Author

veeceey commented Feb 11, 2026

Did the git archaeology. Here's what I found:

The line was introduced in the very first docs commit: 3f214ef ("Add initial docs") by Andrew on 2016-02-07.

In that commit, there are three "inherited from" lines in docs/multidict.rst:

  1. Line 229 (CIMultiDict section): The class is inherited from :class:\MultiDict`.`
  2. Line 330 (CIMultiDictProxy section): The class is inherited from :class:\MultiDict`.`
  3. Line 371 (upstr section): The class is inherited from :class:\str` and has all regular string methods.`

Looking at the actual code at that commit — both the pure Python (_multidict_py.py) and Cython (_multidict.pyx) implementations:

# _multidict_py.py
class CIMultiDict(_CIBase, MultiDict):       # inherits MultiDict ✓
class CIMultiDictProxy(_CIBase, MultiDictProxy):  # inherits MultiDictProxy ✗ (docs say MultiDict)
# _multidict.pyx
cdef class CIMultiDict(MultiDict):            # inherits MultiDict ✓
cdef class CIMultiDictProxy(MultiDictProxy):  # inherits MultiDictProxy ✗ (docs say MultiDict)

So the CIMultiDict line (1) is correct — it does inherit from MultiDict. The CIMultiDictProxy line (2) is a copy-paste from the CIMultiDict section and was wrong from day one. It was always meant to document the base class, not the initializer argument type — the initializer argument is already documented separately on the line above it ("Raises :exc:TypeError if multidict is not :class:CIMultiDict instance.").

The CIMultiDictProxy section in the original commit follows the same structure as CIMultiDict: description, then TypeError note, then "inherited from" line. It was clearly just a copy-paste oversight where MultiDict wasn't updated to MultiDictProxy.

No commits between the initial one and now have touched this specific line — it's been MultiDict (incorrectly) since 2016.

@veeceey
Copy link
Contributor Author

veeceey commented Feb 11, 2026

Yeah, autoclass would definitely be cleaner for documenting inheritance — agreed on that. That feels like a separate discussion/PR though; this one just fixes the factual error in the existing free-form text (MultiDict → MultiDictProxy).

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Feb 12, 2026
Copy link
Member

@Vizonex Vizonex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to the grammar in the documentation looks good and makes sense to me. You'll have to wait for what the others have to say about it but so far I have no comments on what you need to end up changing here.

@Vizonex Vizonex assigned Vizonex and unassigned Vizonex Feb 12, 2026
@Vizonex
Copy link
Member

Vizonex commented Feb 12, 2026

sorry I had a mis-click. I'm just waiting for this PR to be merged at this point unless others have different opinions or ideas.

@veeceey
Copy link
Contributor Author

veeceey commented Feb 16, 2026

Did the git archeology by hand as suggested:

The line The class is inherited from :class:\MultiDict`.forCIMultiDictProxy was introduced in the very first docs commit: [3f214ef`](3f214ef) ("Add initial docs") by Andrew Svetlov on Feb 7, 2016. It was never intentionally changed since -- it's been there untouched for ~10 years.

Looking at the actual MRO:

>>> from multidict import CIMultiDictProxy
>>> CIMultiDictProxy.__mro__
(<class 'CIMultiDictProxy'>, <class '_CIMixin'>, <class 'MultiDictProxy'>, <class '_CSMixin'>, <class 'MultiMapping'>, <class 'Mapping'>, ...)

CIMultiDictProxy inherits from MultiDictProxy, not MultiDict. The original text was simply a copy-paste oversight in the initial docs -- CIMultiDict does inherit from MultiDict, and it looks like that line was duplicated for the Proxy variant without updating the parent class name.

So the fix from MultiDict -> MultiDictProxy is correct. There was no deeper intent behind the original wording; it was just an error from day one.

@veeceey
Copy link
Contributor Author

veeceey commented Feb 17, 2026

Hi @webknjaz, friendly ping — the git archaeology showing the copy-paste origin is all there, and Vizonex has approved. Would appreciate your sign-off when you have a moment!

@veeceey
Copy link
Contributor Author

veeceey commented Feb 18, 2026

Thanks for the discussion @webknjaz and @Dreamsorcerer. I looked into the git history for context on the original wording. The documentation was added in the initial docs setup, and based on the class hierarchy in the source code, CIMultiDictProxy does inherit from MultiDictProxy (not CIMultiDict). The original text saying "is inherited from CIMultiDict" appears to have been an error rather than an intentional description of the initializer argument type.

I'm happy to also explore using .. autoclass:: directives if you'd prefer that approach to document the inheritance hierarchy automatically. Let me know how you'd like to proceed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments