Use Union[bytes, Text] for paths in os.py #1054

euresti · 2017-03-21T04:59:55Z

In order to unify these two versions I'm making all paths be _PathType = Union[bytes, Text]
Fixes #439

In order to unify these two versions I'm making all paths be _PathType = Union[bytes, Text] Fixes python#439

JelleZijlstra · 2017-03-21T05:05:47Z

stdlib/2/os/__init__.pyi

+def symlink(source: _PathType, link_name: _PathType) -> None: ...
+def unlink(path: _PathType) -> None: ...
+def utime(path: _PathType, times: Optional[Tuple[int, int]]) -> None: ...
+def walk(top: _PathType, topdown: bool = ..., onerror: Any = ...,


AnyStr is actually correct here: os.walk returns bytes if you give it bytes and Text if you give it Text. Besides, we should generally avoid Unions in return types.

Oops. Went a bit too far here.

JelleZijlstra · 2017-03-21T05:08:24Z

stdlib/2/os/__init__.pyi

+def execle(file: _PathType, *args) -> None: ...
+def execlp(file: _PathType, *args) -> None: ...
+def execlpe(file: _PathType, *args) -> None: ...
+def execv(path: _PathType, args: Union[Tuple[_PathType], List[_PathType]]) -> None: ...


PathType is an unfortunate name for the args, since they don't have to be Paths.

I think the best solution might be to add a second type alias (or just use Union directly here), so we can easily change the _PathType alias later to include os.PathLike in Python 3.6+.

This also applies to the other exec* function and to the spawn* functions below.

JelleZijlstra · 2017-03-21T05:09:39Z

stdlib/3/os/__init__.pyi

-def execvp(file: AnyStr, args: Union[Tuple[AnyStr], List[AnyStr]]) -> None: ...
-def execvpe(file: AnyStr, args: Union[Tuple[AnyStr], List[AnyStr]],
+def execv(path: _PathType, args: Union[Tuple[_PathType], List[_PathType]]) -> None: ...
+def execve(path: _PathType, args: Union[Tuple[_PathType], List[_PathType]], env: Mapping[_PathType, _PathType]) -> None: ...


Same comments as on the Python 2 version.

JelleZijlstra · 2017-03-21T05:10:22Z

Would it make sense to unify the Python 2 and 3 stubs first, or are the differences still big enough to make that not feasible?

euresti · 2017-03-21T05:25:51Z

Would it make sense to unify the Python 2 and 3 stubs first, or are the differences still big enough to make that not feasible?

I did start with a a full merge but thought splitting it up might be useful. I think my plan is:

_PathType
_StatVFS -> statvfs_result, make _Environs equal
Merge all the comments (Lots of # only Unix)
Merge everything else (adding py3 checks to 2) to make sure tests pass
Rename files

I think that will make it easier to review everything and not miss anything. I'm still pretty new to github so I don't know if splitting up a diff into multiple PRs is useful or not.

JelleZijlstra · 2017-03-21T05:28:01Z

Thanks, that makes sense. Splitting things up definitely makes it easier to review.

JelleZijlstra · 2017-03-21T05:53:58Z

Thanks!

sproshev · 2017-03-21T11:29:58Z

I think os.PathLike introduced in Python 3.6 could be easily supported by changing

_PathType = Union[bytes, Text]

to

if sys.version_info >= (3, 6):
    _PathType = Union[bytes, Text, PathLike]
else:
    _PathType = Union[bytes, Text]

in stdlib/3/os/__init__.pyi

gvanrossum · 2017-03-21T14:40:57Z

Hey @JelleZijlstra Welcome to the crew! Can you please always use Squash Merge to merge PRs? We don't want the contributor's local history preserved forever in our master branch; the history remains available for archeologists in the (closed) PR itself. For this PR what's done is done, but in the future I'd really appreciate it.

(Also, we probably need to write this up somewhere.)

JelleZijlstra · 2017-03-21T14:46:13Z

Oops, sorry for that. I'll squash in the future.

JelleZijlstra · 2017-03-22T15:23:41Z

This caused some errors in mypy's test suite:

FAILURE  #121 check stdlibsamples (3.2)

subprocess.py:1322: error: Argument 2 to "execvp" has incompatible type List[str]; expected "Union[Tuple[Union[bytes, str]], List[Union[bytes, str]]]"
subprocess.py:1324: error: Argument 2 to "execvpe" has incompatible type List[str]; expected "Union[Tuple[Union[bytes, str]], List[Union[bytes, str]]]"

I think mypy is right here: the argument to List is invariant, so List[str] should not be compatible with List[Union[bytes, str]]. We can fix this by making it take Union[Tuple[Union[bytes, str], ...], List[bytes], List[str], List[Union[bytes, str]] instead, so I'll submit a PR to that effect (but not right now).

We can't make these functions take a Sequence[Union[bytes, str]] because the exec* functions really do accept only tuples and lists.

gvanrossum · 2017-03-22T16:41:03Z

Sounds like a plan. Maybe the union should even include List[bytes], List[Text], List[Union[bytes, Text]]? Also please change Tuple[Union[bytes, Text]] to Tuple[Union[bytes, Text], ...] (otherwise only tuples of length 1 are accepted).

euresti · 2017-03-22T17:24:32Z

Oops. Sorry about that. Will send out a PR right now!

JelleZijlstra · 2017-03-22T17:50:42Z

This was fixed in #1075.

It seems that AnyStr has the main advantage of ensuring that multiple arguments (or argument(s) and return values) have the same type (i.e. `str` and `bytes` are both okay but shouldn't be mixed). However it makes it significantly more difficult to cast to a single type. With a Union, something like: ```python def foo(a: Union[bytes, Text]) -> Text: if isinstance(a, bytes): a = a.decode() return "Hello, " + a ``` works fine. With AnyStr, you have to define a new intermediate variable, something like: ```python def foo(a: AnyStr) -> str: if isinstance(a, bytes): b = a.decode() else: b = a return "Hello, " + b ``` If there's no advantage to using AnyStr (since there's no other AnyStr argument or return value), I think the Union would be simpler, have no significant disadvantage, and there is [precedent for similar changes](python#1054). > It's only the case that if there's exactly one parameter of type exactly AnyStr, and no other use of AnyStr in the signature, then Union[str, bytes] should be acceptable. - [gvanrossum](python#439 (comment)) Discussion: - python#1054 - python/mypy#1141 - python#439

Use Union[bytes, Text] in os.pyi

66fbb26

In order to unify these two versions I'm making all paths be _PathType = Union[bytes, Text] Fixes python#439

JelleZijlstra requested changes Mar 21, 2017

View reviewed changes

Missed some that were marked as str

eb5d8c9

Bring some things back to Union types

a4d70e5

David Euresti added 2 commits March 20, 2017 22:29

Bring some things back to Union types

2804788

Fix typo

b362177

JelleZijlstra merged commit 2e1ebab into python:master Mar 21, 2017

euresti deleted the path_type branch March 21, 2017 08:33

JelleZijlstra mentioned this pull request Mar 22, 2017

stubs for xdrlib #1049

Merged

n8henrie mentioned this pull request Jan 9, 2018

Use Union[bytes, Text] instead of AnyStr #1819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Union[bytes, Text] for paths in os.py #1054

Use Union[bytes, Text] for paths in os.py #1054

euresti commented Mar 21, 2017

JelleZijlstra Mar 21, 2017

euresti Mar 21, 2017

JelleZijlstra Mar 21, 2017

euresti Mar 21, 2017

JelleZijlstra Mar 21, 2017

euresti Mar 21, 2017

JelleZijlstra commented Mar 21, 2017

euresti commented Mar 21, 2017

JelleZijlstra commented Mar 21, 2017

JelleZijlstra commented Mar 21, 2017

sproshev commented Mar 21, 2017

gvanrossum commented Mar 21, 2017

JelleZijlstra commented Mar 21, 2017

JelleZijlstra commented Mar 22, 2017 •

edited

Loading

gvanrossum commented Mar 22, 2017

euresti commented Mar 22, 2017

JelleZijlstra commented Mar 22, 2017

Use Union[bytes, Text] for paths in os.py #1054

Use Union[bytes, Text] for paths in os.py #1054

Conversation

euresti commented Mar 21, 2017

JelleZijlstra Mar 21, 2017

Choose a reason for hiding this comment

euresti Mar 21, 2017

Choose a reason for hiding this comment

JelleZijlstra Mar 21, 2017

Choose a reason for hiding this comment

euresti Mar 21, 2017

Choose a reason for hiding this comment

JelleZijlstra Mar 21, 2017

Choose a reason for hiding this comment

euresti Mar 21, 2017

Choose a reason for hiding this comment

JelleZijlstra commented Mar 21, 2017

euresti commented Mar 21, 2017

JelleZijlstra commented Mar 21, 2017

JelleZijlstra commented Mar 21, 2017

sproshev commented Mar 21, 2017

gvanrossum commented Mar 21, 2017

JelleZijlstra commented Mar 21, 2017

JelleZijlstra commented Mar 22, 2017 • edited Loading

gvanrossum commented Mar 22, 2017

euresti commented Mar 22, 2017

JelleZijlstra commented Mar 22, 2017

JelleZijlstra commented Mar 22, 2017 •

edited

Loading