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

Compileall rx argument fix #5172

Merged
merged 10 commits into from
Apr 11, 2021
Merged

Compileall rx argument fix #5172

Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c4b3729
Change compileall rx variable to accept a protocol
hatal175 Apr 1, 2021
a164ab4
Removing unused import
hatal175 Apr 1, 2021
3ce864b
Trying to use typeshed typevar
hatal175 Apr 1, 2021
71be86d
Trying to extract SupportsSearch out of file
hatal175 Apr 1, 2021
b86eef9
Remove unused import
hatal175 Apr 1, 2021
1b0748b
Remove another unused import
hatal175 Apr 1, 2021
b38f5f4
Move SupportsSearch back to compileall with Protocol imported from ty…
hatal175 Apr 2, 2021
04f2320
Remove unused import
hatal175 Apr 2, 2021
b743c91
Merge branch 'master' into compileall
hatal175 Apr 10, 2021
c57e3c4
Change AnyPath to StrPath
hatal175 Apr 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions stdlib/compileall.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
import sys
from _typeshed import AnyPath
from typing import Any, Optional, Pattern
from _typeshed import AnyPath, _T_contra
from typing import Any, AnyStr, Optional, Protocol

if sys.version_info >= (3, 7):
from py_compile import PycInvalidationMode

class _SupportsSearch(Protocol[_T_contra]):
def search(self, string: _T_contra) -> Any: ...

if sys.version_info >= (3, 9):
def compile_dir(
dir: AnyPath,
maxlevels: Optional[int] = ...,
ddir: Optional[AnyPath] = ...,
force: bool = ...,
rx: Optional[Pattern[Any]] = ...,
rx: Optional[_SupportsSearch[AnyStr]] = ...,
Copy link
Member

Choose a reason for hiding this comment

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

The AnyStr doesn't make sense to me here; how is the typevar going to get constrained? I'm guessing it depends on whether dir is a bytes or a str path, so perhaps that argument should also have a TypeVar in it.

(Note that AnyPath and AnyStr are very different things: the first is a union and the second a TypeVar.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, so compile_dir runs compile_file on every file it can using listdir - which means file will be bytes or str, so rx there can accept str and bytes. which is why I used AnyStr, a type that can be either or.

compile_file however gets AnyPath as file name and can pass that along to rx as is if quiet is 2. Meaning that the rx in compile_file might need to handle the same type as fullname. Which means I might need to fix that? Not sure what to do there.

Either way, I'm not following you. What did you have in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Well, the place I started with is that having a single TypeVar in a signature tends to be a mistake, because the point of a typevar is to use the same type in multiple places. See the discussion in https://mail.python.org/archives/list/typing-sig@python.org/message/NRFNHGXHXPGBR6FP3TIOZZ6VS4XJZX6K/ for some context.

But I just tried it out and at least in 3.7, we don't need all these Unions: compile_dir actually only works on str paths, not bytes paths. In compile_file, it calls importlib.util.cache_from_source, and that only accepts str, not bytes. So instead, let's just change all the parameters dealing with paths to accept only strs.

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 changed things to StrPath since it does work with PathLike[str].

quiet: int = ...,
legacy: bool = ...,
optimize: int = ...,
Expand All @@ -27,7 +30,7 @@ if sys.version_info >= (3, 9):
fullname: AnyPath,
ddir: Optional[AnyPath] = ...,
force: bool = ...,
rx: Optional[Pattern[Any]] = ...,
rx: Optional[_SupportsSearch[AnyStr]] = ...,
quiet: int = ...,
legacy: bool = ...,
optimize: int = ...,
Expand All @@ -45,7 +48,7 @@ elif sys.version_info >= (3, 7):
maxlevels: int = ...,
ddir: Optional[AnyPath] = ...,
force: bool = ...,
rx: Optional[Pattern[Any]] = ...,
rx: Optional[_SupportsSearch[AnyStr]] = ...,
quiet: int = ...,
legacy: bool = ...,
optimize: int = ...,
Expand All @@ -56,21 +59,20 @@ elif sys.version_info >= (3, 7):
fullname: AnyPath,
ddir: Optional[AnyPath] = ...,
force: bool = ...,
rx: Optional[Pattern[Any]] = ...,
rx: Optional[_SupportsSearch[AnyStr]] = ...,
quiet: int = ...,
legacy: bool = ...,
optimize: int = ...,
invalidation_mode: Optional[PycInvalidationMode] = ...,
) -> int: ...

else:
# rx can be any object with a 'search' method; once we have Protocols we can change the type
def compile_dir(
dir: AnyPath,
maxlevels: int = ...,
ddir: Optional[AnyPath] = ...,
force: bool = ...,
rx: Optional[Pattern[Any]] = ...,
rx: Optional[_SupportsSearch[AnyStr]] = ...,
quiet: int = ...,
legacy: bool = ...,
optimize: int = ...,
Expand All @@ -80,7 +82,7 @@ else:
fullname: AnyPath,
ddir: Optional[AnyPath] = ...,
force: bool = ...,
rx: Optional[Pattern[Any]] = ...,
rx: Optional[_SupportsSearch[AnyStr]] = ...,
quiet: int = ...,
legacy: bool = ...,
optimize: int = ...,
Expand Down