Update and add stubs for os module #1645

hashstat · 2017-10-03T19:51:25Z

There were many missing methods and many that were updated in Python 3.3 and later. This pull request should bring the os module up-to-date.

JelleZijlstra

Thanks for making all of these fixes! This will make the os stubs a lot better.

JelleZijlstra · 2017-10-05T03:52:06Z

stdlib/3/os/__init__.pyi

 def getgroups() -> List[int]: ...  # Unix only, behaves differently on Mac
 def initgroups(username: str, gid: int) -> None: ...  # Unix only
 def getlogin() -> str: ...
 def getpgid(pid: int) -> int: ...  # Unix only
 def getpgrp() -> int: ...  # Unix only
 def getpid() -> int: ...
 def getppid() -> int: ...
+if sys.version_info >= (3, 3):
+    def getpriority(which: int, who: int) -> int: ...  # Unix only
+    def setpriority(which: int, who: int, priority: int) -> int: ...  # Unix only


Actually returns None.

JelleZijlstra · 2017-10-05T03:56:31Z

stdlib/3/os/__init__.pyi

 def isatty(fd: int) -> bool: ...  # Unix only
+if sys.version_info >= (3, 3):
+    def lockf(fd: int, cmd: int, length: int) -> None: ...  # Unix only


The code calls the second argument command, but it's positional-only anyway.

JelleZijlstra · 2017-10-05T03:57:42Z

stdlib/3/os/__init__.pyi

 def lseek(fd: int, pos: int, how: int) -> int: ...
-def open(file: _PathType, flags: int, mode: int = ...) -> int: ...
+if sys.version_info >= (3, 3):
+    def open(file: _PathType, flags: int, mode: Optional[int] = ..., *, dir_fd: Optional[int] = ...) -> int: ...


The mode argument is not Optional.

JelleZijlstra · 2017-10-05T04:03:23Z

stdlib/3/os/__init__.pyi

 def read(fd: int, n: int) -> bytes: ...
+if sys.version_info >= (3, 3):
+    # sendfile() has a different variation on FreeBSD and Mac OS X
+    def sendfile(out_fd: int, in_fd: int, offset: Optional[int], count: int) -> int: ...  # Unix only


If I'm reading the code right count is also Optional on at least some variations. I think we should give the most permissive type.

You're also missing the arguments headers, trailers, and flags. headers and trailers are Sequences of bytes and flags is an int.

I didn't have a Mac on which to test the variant supporting headers, trailers, and flags, so I left it out. But I downloaded a FreeBSD image and added an overloaded variant. When testing, I found that count is not Optional on Linux or FreeBSD.

JelleZijlstra · 2017-10-05T04:08:54Z

stdlib/3/os/__init__.pyi

+    def writev(fd: int, buffers: Sequence[bytes]) -> int: ...  # Unix only
+
+    terminal_size = NamedTuple('terminal_size', [('columns', int), ('lines', int)])
+    def get_terminal_size(fd: Optional[int] = ...) -> terminal_size: ...


In [9]: os.get_terminal_size(None) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-9-46d26e904443> in <module>() ----> 1 os.get_terminal_size(None) TypeError: an integer is required (got type NoneType)

So it's not Optional.

Another place where I was overzealous with Optionals. I think I fixed them all.

JelleZijlstra · 2017-10-05T04:12:05Z

stdlib/3/os/__init__.pyi

-def chdir(path: _PathType) -> None: ...
+if sys.version_info >= (3, 3):
+    def access(path: _FdOrPathType, mode: int, *, dir_fd: Optional[int] = ...,
+               effective_ids: Optional[bool] = ..., follow_symlinks: Optional[bool] = ...) -> bool: ...


I think these two are supposed to be just bool, not Optional[bool], though in reality it will accept anything and call bool() on it.

JelleZijlstra · 2017-10-05T04:13:27Z

stdlib/3/os/__init__.pyi

 def fchdir(fd: int) -> None: ...
 def getcwd() -> str: ...
 def getcwdb() -> bytes: ...
-def chflags(path: _PathType, flags: int) -> None: ...  # Unix only
+if sys.version_info >= (3, 3):
+    def chflags(path: _PathType, flags: int, *, follow_symlinks: Optional[bool] = ...) -> None: ...  # Unix only


The last arguments is not actually keyword-only (and I think should not be Optional). The other boolean arguments below should not be Optional either.

Sure enough. I tried to test this one, but it isn't supported on Linux. My new FreeBSD VM image solved that for me.

JelleZijlstra · 2017-10-05T04:15:09Z

stdlib/3/os/__init__.pyi

+    def listdir(path: Optional[str] = ...) -> List[str]: ...
+    @overload
+    def listdir(path: bytes) -> List[bytes]: ...
+    @overload


Can't you just define only this overload in a conditional block?

That's what I thought, but mypy complains when I do that:

stdlib/3/os/__init__.pyi:432: error: Name 'listdir' already defined stdlib/3/os/__init__.pyi:432: error: Single overload definition, multiple required

JelleZijlstra · 2017-10-05T04:16:56Z

stdlib/3/os/__init__.pyi

-def mknod(filename: _PathType, mode: int = ..., device: int = ...) -> None: ...
+if sys.version_info >= (3, 3):
+    def lstat(path: _PathType, *, dir_fd: Optional[int] = ...) -> stat_result: ...
+    def mkdir(path: _PathType, mode: Optional[int] = ..., *, dir_fd: Optional[int] = ...) -> None: ...


These mode arguments are not Optional. More instances of the same below: mode arguments should be int, dir_fd is correct as Optional[int], and all the boolean args should just be bool.

JelleZijlstra · 2017-10-05T04:20:17Z

stdlib/3/os/__init__.pyi

-def unlink(path: _PathType) -> None: ...
-if sys.version_info >= (3, 0):
-    def utime(path: _PathType, times: Optional[Union[Tuple[int, int], Tuple[float, float]]] = ...,
+if sys.version_info >= (3, 3):


Didn't read further down from here yet.

Also fixes other issue found during review.

hashstat · 2017-10-05T15:56:11Z

Thanks for looking at this so thoroughly. I know it's a big change and I messed it up with the ideas that were stuck in my head regarding Optionals.

JelleZijlstra

This time I made it all the way through. I noticed a few more issues; thanks for fixing the previous batch!

JelleZijlstra · 2017-10-09T03:57:41Z

stdlib/3/os/__init__.pyi

@@ -375,6 +531,16 @@ else:
    def walk(top: AnyStr, topdown: bool = ..., onerror: Optional[Callable[[OSError], Any]] = ...,
             followlinks: bool = ...) -> Iterator[Tuple[AnyStr, List[AnyStr],
                                                        List[AnyStr]]]: ...
+if sys.version_info >= (3, 3):
+    def fwalk(top: AnyStr = ..., topdown: bool = ...,


top is also allowed to be an int or a Path, or not given (in which case it's a str). I think we need some overloads for those cases.

So it accepts a file descriptor, but I get an error:

>>> next(os.fwalk(fd)) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib/python3.6/os.py", line 458, in fwalk orig_st = stat(top, follow_symlinks=False, dir_fd=dir_fd) ValueError: stat: cannot use fd and follow_symlinks together

I also don't see anything in the documentation suggesting that top can be an int.

Interesting, the implementation explicitly checks for isinstance(top, int). Maybe this is a bug in CPython.

JelleZijlstra · 2017-10-09T04:00:12Z

stdlib/3/os/__init__.pyi

+              onerror: Optional[Callable] = ..., *, follow_symlinks: bool = ...,
+              dir_fd: Optional[int] = ...) -> Iterator[Tuple[AnyStr, List[AnyStr],
+                                             List[AnyStr], int]]: ...  # Unix only
+    def getxattr(path: _FdOrPathType, attribute: AnyStr, *, follow_symlinks: bool = ...) -> bytes: ...  # Linux only


AnyStr generally doesn't make sense if it's only used once, since it's meant to express that multiple values should be the same type (either str or bytes). Also, the argument clinic types suggest these "attributes" are paths, so maybe PathLike should also be allowed.

Yeah... sorry. Interesting thing is that the documentation says that fwalk() "behaves exactly like walk()", but that's not the case. If walk() is called with a bytes path, the results are also bytes, hence the use of AnyStr. However, fwalk() returns strings regardless of the input path type.

JelleZijlstra · 2017-10-09T04:00:59Z

stdlib/3/os/__init__.pyi

-        def __init__(self, command: str, mode: str = ...,
-                     bufsize: int = ...) -> None: ...
-        def close(self) -> Any: ...  # may return int
-else:


Let's leave this in so I can finish my project to merge the Python 2 and 3 os stubs.

JelleZijlstra · 2017-10-09T04:04:06Z

stdlib/3/posix.pyi

@@ -10,3 +10,23 @@ from typing import NamedTuple
 if sys.version_info >= (3, 3):
    uname_result = NamedTuple('uname_result', [('sysname', str), ('nodename', str),
        ('release', str), ('version', str), ('machine', str)])
+
+    times_result = NamedTuple('times_result', [


Not sure why these specifically are in this file; most of the functions also live in the posix module. Eventually we should move most code into posix.pyi and thus re-export it in os.pyi.

Sounds like a good idea. I was just keeping with the convention used with uname_result.

Address specific code review comments.

Update and add stubs for os module

07916eb

hashstat force-pushed the os-module-updates branch from 754a1cf to 07916eb Compare October 3, 2017 20:01

JelleZijlstra requested changes Oct 5, 2017

View reviewed changes

Fix Optional arguments

8bac723

Also fixes other issue found during review.

JelleZijlstra mentioned this pull request Oct 6, 2017

Some os and os.path function signatures do not admit legal integer (file descriptor) arguments #1653

Closed

JelleZijlstra requested changes Oct 9, 2017

View reviewed changes

Additional type annotation fixes

e81eb70

Address specific code review comments.

JelleZijlstra approved these changes Oct 10, 2017

View reviewed changes

JelleZijlstra merged commit d333474 into python:master Oct 10, 2017

hashstat deleted the os-module-updates branch October 25, 2017 17:34

FichteFoll mentioned this pull request Jan 24, 2018

State of os.path and path types #1841

Closed

Uh oh!

Update and add stubs for os module #1645

Update and add stubs for os module #1645

Uh oh!

Conversation

hashstat commented Oct 3, 2017

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashstat commented Oct 5, 2017

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!