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

Support for pexpect.spawn(..., logfile=sys.stdout) #10976

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

PokkaKiyo
Copy link
Contributor

@PokkaKiyo PokkaKiyo commented Nov 4, 2023

We can set the logfile for pexpect's spawn using sys.stdout, but the type hint shows an error.

This comment has been minimized.

@PokkaKiyo PokkaKiyo changed the title Support for spawn(..., logfile=sys.stdout) Support for pexpect.spawn(..., logfile=sys.stdout) Nov 4, 2023
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.

Thanks! Looking at the source code, it might be better to just define a custom protocol, which we can use for both SpawnBase and spawn:

class _Logfile(Protocol):
    def write(self, __s: str) -> object:...
    def flush(self) -> object: ...

Source:

https://github.com/pexpect/pexpect/blob/c53497663c1a25603ccf8ba7daca03b319387344/pexpect/spawnbase.py#L128-L130

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 6, 2023

Thanks! Looking at the source code, it might be better to just define a custom protocol, which we can use for both SpawnBase and spawn:

class _Logfile(Protocol):
    def write(self, __s: str) -> object:...
    def flush(self) -> object: ...

We already define a protocol like that elsewhere in typeshed. Maybe we should add it to stdlib/_typeshed/__init__.pyi?:

typeshed/stdlib/builtins.pyi

Lines 1640 to 1641 in 6c5d1d3

class _SupportsWriteAndFlush(SupportsWrite[_T_contra], Protocol[_T_contra]):
def flush(self) -> None: ...

EDIT: Actually, no, because we wouldn't be able to use that in our pexpect stubs until we've had a mypy release that includes the updates to the _typeshed directory in its vendored stubs. So we should just copy that protocol into our pexpect stubs, like @srittau suggests.

@srittau
Copy link
Collaborator

srittau commented Nov 6, 2023

We already define a protocol like that elsewhere in typeshed. Maybe we should add it to stdlib/_typeshed/init.pyi?

While I would be ok with that, I am not a huge fan of adding all sometimes-used permutations of I/O methods to _typeshed. I'd rather just switch to intersection types (python/typing#213) at some point in the far future. That said, moving the protocol to _typeshed could help use find places where we could use intersection types in the future.

(Also, flush() should return object, not None.)

Copy link
Contributor

github-actions bot commented Nov 9, 2023

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

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.

Thanks!

@srittau srittau merged commit 7f9b3ea into python:main Nov 9, 2023
48 checks passed
@PokkaKiyo PokkaKiyo deleted the pexpect-spawn-logfile-type branch November 9, 2023 11:25
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.

None yet

3 participants