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

Type openpyxl functions module and its usages #10322

Merged
merged 8 commits into from
Jul 19, 2023

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Jun 16, 2023

Split off from, and improved over, #9511

  • Typed openpyxl functions module and its usages
    Typing this module is kind of a real mess, in-code comment should explain it well. Since we can't really use any other xml libraries in these stubs anyway for various reasons (untyped, not a dependency, ...), xml is the only compatible one for typing openpyxl.
  • Still I tried to use sensible Protocols when possible. For that reason I also moved some shared protocols from nested.pyi.
  • Created _functions_overloads.pyi to abstract away most of the complexity and re-implement overloads of the different possible libraries.
  • Updated the stubtest_allowlist

@Avasam Avasam changed the title Type openpyxl functions module and its usages Type openpyxl functions module and its usages Jun 16, 2023
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Comment on lines 11 to 13
out: IO[str] | str
xf: Incomplete
def __init__(self, ws, out: Incomplete | None = None) -> None: ...
def __init__(self, ws, out: IO[str] | str | None = None) -> None: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

We try to avoid IO etc. in typeshed if possible. Is it possible to use a protocol here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the close and write methods are used. (Maybe more I'll double check usage in other parts of the code)
If the out parameter is None, it seems to default to a BufferedWriter and IO was the top parent with both methods.

Copy link
Collaborator Author

@Avasam Avasam Jun 17, 2023

Choose a reason for hiding this comment

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

I simplified it down to StrPath | str, I changed it to StrPath with an additional union to BytesIO because there's an explicit if isinstance(self.out, BytesIO) branch in WorksheetWriter.read.

Copy link
Member

Choose a reason for hiding this comment

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

StrPath is a union that already includes str, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

uh, yes, lemme simplify that too real quick ^^

Copy link
Collaborator Author

@Avasam Avasam Jun 17, 2023

Choose a reason for hiding this comment

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

Looking at the et_xmlfile implementation of xmlfile, if not passing directly a string, it needs the close and write method. But that one isn't restricted to BytesIO. Maybe I should just replace BytesIO in that union with a protocol that has both methods and that BytesIO can match.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

LGTM, at least as a first stab. Original code is a mess, so let's see whether there are any problems in the future.


# et_xmlfile.xmlfile accepts a str | _SupportsCloseAndWrite
# lxml.etree.xmlfile should accept a StrPath | _SupportsClose https://lxml.de/api/lxml.etree.xmlfile-class.html
_OutType: TypeAlias = _SupportsCloseAndWrite | StrPath
Copy link
Collaborator

@srittau srittau Jun 28, 2023

Choose a reason for hiding this comment

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

Yikes, that code is a mess in the original source code. So if I read this correctly, WorksheetWriter.read() only works with BytesIO and whatever can be passed to open() (str-like paths). .cleanup() work only with str-like paths. .get_stream() calls xmlfile(), where at least the lxml variant needs a "file-like object" or a string, according to the docs. And, if not supplied to the constructor, it's initialized to NamedTemporaryFile() (or its return value), which is neither a str-like path nor a BytesIO. Yeah, let's just use your solution for now.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2023

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

@srittau srittau merged commit 6191425 into python:main Jul 19, 2023
49 checks passed
@Avasam Avasam deleted the openpyxl-functions-module-and-usage branch July 20, 2023 04:50
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.

3 participants