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.

Sign up for GitHub

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

Jump to bottom

Fix CLOSE_STDIN sentinel #6760

Closed
wants to merge 2 commits into from
Closed

Fix CLOSE_STDIN sentinel #6760

wants to merge 2 commits into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Feb 19, 2020 •
edited
Loading

This makes it / it's repr more meaningful in docs, and should have been
object() anyway.

TODO:

@blueyed blueyed mentioned this pull request Feb 19, 2020
Merged
bluetech
bluetech approved these changes Feb 28, 2020
View reviewed changes
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

LGTM.

I wonder if might be worth it to solve this centrally, maybe handrolling a version of unittest.mock.sentinel: https://github.com/python/cpython/blob/e263bb1e97ae8f84fb4f2ab5b0c4f529a2e5696d/Lib/unittest/mock.py#L276-L307

src/_pytest/pytester.py Outdated
@@ -534,7 +534,8 @@ class Testdir:

__test__ = False

CLOSE_STDIN = object
class CLOSE_STDIN:
pass
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pass
__slots__ = ()

Copy link
Member

Choose a reason for hiding this comment

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

Hmm given that you use the class itself as the sentinel and not an instance of it, this suggestion doesn't matter much...

Copy link
Member

@graingert graingert Feb 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

an enum instance is usually pretty good

class CloseStdinType(enum.Enum):
    CLOSE_STDIN = enum.auto()

CLOSE_STDIN = CloseStdinType.CLOSE_STDIN

it's great for type annotations too

Copy link
Member

Choose a reason for hiding this comment

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

and it's got that None/NoneType feel

Copy link
Member

@graingert graingert Feb 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@blueyed it enables type annotations like:

def popen(self, ..., stdin=CLOSE_STDIN: Union[CloseStdinType,bytes,int]): -> ...

Copy link
Member

Choose a reason for hiding this comment

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

@graingert Right, this is the best current way to do it in type-compatible way: python/typing#689. Also what I opted for here: bluetech@bcd19fa#diff-4ec37b1ad5b7d41fdac56c907d08dde1R46-R52

Copy link
Contributor Author

@blueyed blueyed Mar 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What's wrong with stdin: Optional[Union[…, Type[CLOSE_STDIN]]] = CLOSE_STDIN,?
It might be not as strict really, but serves the purpose in general, and has less/no boilerplate.

Copy link
Member

Choose a reason for hiding this comment

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

Nothing wrong really, just that using a Type[...] as a sentinel is odd/surprising. I'd go with @graingert's suggestion, which is the "official" way to do it.

That said if you prefer the Type that's fine.

@blueyed
Copy link
Contributor Author

blueyed commented Feb 28, 2020

maybe handrolling a version of unittest.mock.sentinel

I'm not opposed to it.

Given that this needs to be rebased anyway suggestions are welcome.
OTOH we're not using it that much currently, and therefore I think it's fine to go with it for now.

@blueyed blueyed force-pushed the pytester-CLOSE_STDIN-upstream branch from 0dd5677 to d8a42f8 Compare February 28, 2020 17:45
@blueyed blueyed requested a review from bluetech March 5, 2020 05:44
@blueyed blueyed changed the title Use a class for CLOSE_STDIN sentinel Fix CLOSE_STDIN sentinel Mar 6, 2020
@blueyed blueyed force-pushed the pytester-CLOSE_STDIN-upstream branch 3 times, most recently from 6dfee78 to 64428a1 Compare March 7, 2020 00:16
@blueyed blueyed mentioned this pull request Mar 7, 2020
Merged
@blueyed blueyed force-pushed the pytester-CLOSE_STDIN-upstream branch 2 times, most recently from 133aa67 to 07c05a9 Compare March 7, 2020 00:37
@blueyed
Use a class for CLOSE_STDIN sentinel
4afe98c 
This makes it / it's repr more meaningful in docs, and should have been
`object()` anyway.
@blueyed
Use enum, typing/docs for popen/run
2fdf0f5
@blueyed blueyed force-pushed the pytester-CLOSE_STDIN-upstream branch from 07c05a9 to 2fdf0f5 Compare March 7, 2020 22:53
bluetech
bluetech reviewed Mar 9, 2020
View reviewed changes
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

LGTM, just a few questions.

src/_pytest/pytester.py
@@ -513,7 +519,7 @@ def restore(self) -> None:
sys.path[:], sys.meta_path[:] = self.__saved


class Testdir:
class Testdir(Generic[AnyStr]):
Copy link
Member

Choose a reason for hiding this comment

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

I don't see AnyStr mentioned elsewhere, why did you need this generic?

src/_pytest/pytester.py
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
stdin=CLOSE_STDIN,
stdout: Optional[Union[int, IO]] = subprocess.PIPE,
Copy link
Member

Choose a reason for hiding this comment

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

Is it specifically IO[str] or IO[bytes]?

I see that typeshed punts on this: https://github.com/python/typeshed/blob/d8b081130d3e3a7fde1a49ac787d304f5c4c1966/stdlib/3/subprocess.pyi#L13-L27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used AnyStr here initially (but forgot to clean it up).
I've figured IO is the same as IO[Any]. Do you prefer the latter?
We basically have the _FILE from typeshed here: Union[None, int, IO[Any]]

Copy link
Member

Choose a reason for hiding this comment

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

I'd leave it with the explicit Any (like you did), maybe can be filled with something precise in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be str or bytes. typeshed binds it according to overloads.

src/_pytest/pytester.py
stdin=CLOSE_STDIN,
stdout: Optional[Union[int, IO]] = subprocess.PIPE,
stderr: Optional[Union[int, IO]] = subprocess.PIPE,
stdin: Optional[Union[CloseStdinType, bytes, int, IO]] = CLOSE_STDIN,
Copy link
Member

Choose a reason for hiding this comment

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

Does Literal[CLOSE_STDIN] work? If yes, that'd be a little nicer. (I think it is planned to work, but I don't remember if it does already).

@nicoddemus
Copy link
Member

Closing for now.

@nicoddemus nicoddemus closed this May 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluetech bluetech bluetech approved these changes

@graingert graingert graingert left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@blueyed @nicoddemus @graingert @bluetech