Added missing inline types for public api #7068
Conversation
There was a problem hiding this comment.
Uh oh, the wave of typing has hit PyInstaller. Warning in advance, I really dislike typing for Python so whilst I'm happy for the public API to gain basic (i.e. datas: list rather than datas: List[Tuple[Union[str, os.PathLike], str]]) hints to make IDEs smarter, I don't want it anywhere else and I don't want mypy or any derivatives in PyInstaller.
Can I ask that you remove types for the stuff listed in __all__s - they're not public API. In particular, this change shouldn't touch any of the hooks (PyInstaller/hooks/hook-*.py) or modulegraph (PyInstaller/lib/modulegraph). Likewise with anything in PyInstaller.utils.hooks and Pyinstaller.compat that isn't documented (and yes, I know our public/private API scoping is a mess). Can I also ask that you resist the urge to reformat the copyright headers and leave import order alone.
Avasam
force-pushed
the
py.typed
branch
4 times, most recently
from
f937e6b
to
a8f12a8
Compare
|
I was still midway through configuring my editor for this project (like changing the autoformatter from autopep8 to yapf, disabling import sorting, etc.). And I wanted to see what I had to deal with for PyInstaller's CI. So those unrelated change were not intended to stay. Thanks for the fair warning nonetheless. I've made sure to not use any type var or generic type argument. So any type added is Python 3.7 compatible, directly inlined and at its simplest (other than unions), although partial and incomplete. |
Avasam
force-pushed
the
py.typed
branch
2 times, most recently
from
0951410
to
14a8440
Compare
Avasam
changed the title
PyInstaller/__main__.py
Outdated
| @@ -146,7 +147,7 @@ def generate_parser() -> _PyiArgumentParser: | |||
| return parser | |||
|
|
|||
|
|
|||
| def run(pyi_args=None, pyi_config=None): | |||
| def run(pyi_args: list | None = None, pyi_config: dict | list | None = None): | |||
There was a problem hiding this comment.
pyi_config isn't allowed to be a list.
There was a problem hiding this comment.
Technically it can be an Iterable[tuple[str, object]] (as it's used to update a dict). But as you mentioned in another comment, doesn't mean that's what it's intended for. So I've updated it and other annotations meant to update CONF as well.
PyInstaller/utils/hooks/__init__.py
Outdated
| @@ -746,7 +757,7 @@ def is_module_or_submodule(name, mod_or_submod): | |||
| ] | |||
|
|
|||
|
|
|||
| def collect_dynamic_libs(package, destdir=None): | |||
| def collect_dynamic_libs(package: str, destdir: object | None = None): | |||
There was a problem hiding this comment.
destdir should be a string rather than an arbitrary object.
PyInstaller/utils/hooks/__init__.py
Outdated
| def collect_data_files( | ||
| package: str, | ||
| include_py_files: bool = False, | ||
| subdir: str | bytes | os.PathLike | None = None, |
There was a problem hiding this comment.
Can we just eliminate bytes from all filename type annotations. We never intend these functions to work with bytes.
Avasam
requested review from
bwoodsend and
rokm
and removed request for
bwoodsend
Added type stubs for the generated / fake-module
pyi_splash.I've also completed inline types for all the modules meant to be used in code. So that type checkers don't see the arguments or return types as fully
Unknown. Taken from:I mostly typed function params and class attributes. Then let type inference do the heavy lifting for return types.
To help validate the types, this was first run against TypeShed's CI: Avasam/typeshed#1 (then simplified to their base and non generic types).