Skip to content

Conversation

Screwtapello
Copy link
Contributor

This doesn't add complete type annotations, but it improves things for the most commonly used objects in the SAX API.

It turns out SAX's definition of a "qname" is exactly the opposite of
ElementTree's. With that understanding, let's annotate the Attributes*Impl
classes too.
@github-actions

This comment has been minimized.

@Screwtapello
Copy link
Contributor Author

It looks like a bunch of CI tests failed because third-party-library stubs that subclass xml.sax.handler.ContentHandler and override startElement() don't actually use the correct annotations for the attrs parameter.

  • the docs describe an interface called "Attributes", which is superset of the Mapping[str, str] interface, but no such class exists in the code (probably because the code pre-dates ABCs)
  • the implementation always uses an instance of AttributesImpl, which implements the "Attributes" interface, but of course doesn't actually declare it anywhere (or mention Mapping).
  • My type annotation says the attrs parameter will always be of type AttributesImpl (which is true) but of course that conflicts with everyone that just read the documentation and said "sure, Mapping[str, str] is close enough"

I'm not sure what clever type machinery exists to solve this (type aliases? ABCs? Protocols?) but I'm happy to try things if someone can provide guidance.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, I think your solution makes sense.

@JelleZijlstra JelleZijlstra merged commit 49b717c into python:main Sep 23, 2023
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

Successfully merging this pull request may close these issues.

2 participants