Skip to content

[et_xmlfile] Complete the writer_cm annotation#16075

Open
eddiesr93 wants to merge 1 commit into
python:mainfrom
eddiesr93:et_xmlfile-writer-cm
Open

[et_xmlfile] Complete the writer_cm annotation#16075
eddiesr93 wants to merge 1 commit into
python:mainfrom
eddiesr93:et_xmlfile-writer-cm

Conversation

@eddiesr93

Copy link
Copy Markdown

xmlfile.writer_cm was Incomplete. It is initialised to None in __init__ and assigned the result of the @contextmanager-decorated incremental_tree._get_writer in xmlfile.__enter__, so its type is _GeneratorContextManager[tuple[ET._FileWrite, str]] | None.

The context manager yields (write, encoding): the first element is passed straight to _IncrementalFileWriter, whose output_file parameter is already typed as ET._FileWrite, and the second is the encoding str.

Verified locally with stubtest, mypy, and pyright.

@github-actions

Copy link
Copy Markdown
Contributor

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

class xmlfile:
encoding: str
writer_cm: Incomplete
writer_cm: _GeneratorContextManager[tuple[ET._FileWrite, str]] | None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we can actually type the first tuple element a bit more accurate by using:

Suggested change
writer_cm: _GeneratorContextManager[tuple[ET._FileWrite, str]] | None
writer_cm: _GeneratorContextManager[tuple[SupportsWrite[str], str]] | None

(SupportsWrite must be imported from _typeshed.) _FileWrite is a type alias, of which one element is SupportsWrite. But the other elements of this type alias don't make much sense if you look at how its used:

https://foss.heptapod.net/openpyxl/et_xmlfile/-/blob/branch/default/et_xmlfile/xmlfile.py?ref_type=heads#L91

This will actually just call the writer with a str.

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