-
Notifications
You must be signed in to change notification settings - Fork 214
Add type hints #467
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.
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
base: master
Are you sure you want to change the base?
Add type hints #467
Conversation
package_data={'': ['*.dat', '*.crt']}, | ||
python_requires='>=3.7', | ||
install_requires=[ | ||
'importlib_resources >= 1.3 ; python_version < "3.9"', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type checkers don't have great support for the try
/except
style of providing backwards compatibility. So instead of relying on pkg_resources
, which may be missing even for Python versions 3.7 and 3.8 if a tool like uv
was used to setup the environment, it seemed better to just rely on the importlib_resources
backport with the minimum version that supports the files
function. That way we don't have a soft-dependency on setuptools
.
return self._re.match(number) | ||
|
||
def replace(self, f, b, u, p): | ||
def replace(self, f: str, b: str, u: str, p: str) -> str: | ||
items = iter([f, b, u, p]) | ||
return re.sub(r'([FBUP])\1*', lambda x: next(items), self._fmt) | ||
|
||
|
||
# Convert the structure to something that we can easily use | ||
_number_formats_per_region = dict( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a nicer solution for this other than leaving both _number_formats_per_region_raw
and _number_formats_per_region
around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could del _number_formats_per_region_raw
after it has served its purpose. Or just immediately perform the conversion in a single statement. Although REGIONS
containing the uncleaned region names would make that a little tricky.
|
||
TYPE_CHECKING = False | ||
if TYPE_CHECKING: | ||
from collections.abc import Generator as Generator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand why this code is here. I don't think there is any situation where you can have this if branch be executed.
This code appears to have another origin and more generic than is needed for python-stdnum, where did it come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch is for type checkers only, conversely the else
branch is never looked at by type checkers.
The entire point of this file is to defer the typing
import, since it is quite expensive, which would make python-stdnum
less attractive in embedded environments. But the magic for this is too dynamic for type checkers to be able to understand it, so this branch basically summarizes for type checkers what typing
symbols this module re-exports.
We could avoid the branch by shipping a separate _typing.pyi
file, but it seems easier to keep the typing-only branch in sync with the runtime branch if it's all in the same file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still struggling with getting this merged. The whole _typing.py
file feels like a pretty big hack just to get some type annotations. I will probably merge some bits of this earlier. I'm in the process of extracting some parts into separate commits which leaves the huge one with just mechanical changes.
Are imports from collections.abc
also expensive? If there is an efficient way to be able to reference Any
we can probably make something that is has minimal runtime impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collections.abc
is not as bad as far as overhead is concerned I believe. I just think it's easier if all the types are collected in a common utility module when python-stdnum
relies on such a small number of them. There's less confusion overall if there's only one place to import types from.
The only real friction I see with this approach is, that if you need a type that hasn't been added to _typing.py
yet, you need to add it to both branches in the correct way. But this should be rare, given how much code there is and how little need there is for anything fancy typing-wise.
There are however alternative approaches. We could simplify things, if you don't care to support runtime introspection of type hints at all we could just put all the typing only imports in a if TYPE_CHECKING
block at the start of each file that needs it. This should only really negatively impact users of something like beartype.
There are however still things with runtime effects like cast
and @deprecated
, so for those alone we do need something like _typing.py
, so we can provide our own implementation that doesn't depend on typing
, but for type checkers to be able to understand what these functions do, we need to pretend it's their implementation, not ours.
It is unfortunately what you have to do currently if you want to minimize the runtime impact of type annotations. If you don't care to minimize the runtime impact we can add typing_extensions
as a dependency and import everything at runtime.
Other than that the only option with no runtime overhead is separate stub files. But then you lose both runtime introspection of the type hints and the ability to type check the implementation. So it will not help you find any bugs in stdnum
, just in code that uses it. So at that point it would probably be more ergonomic to contribute the stubs to typeshed, than maintain them in this project and deal with the hassle of updating two separate files each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also just in case this wasn't clear: The if TYPE_CHECKING:
part isn't the hack. typing.TYPE_CHECKING
exists as a constant and it's very commonly used to get around limitations of the type system. Doing TYPE_CHECKING = False
is the currently supported way to avoid the typing
import, since type checkers will still treat it like typing.TYPE_CHECKING
(False
at runtime, True
at type checking time).
The only real hack is the module-level __getattr__
and providing our own implementation for deprecated
/cast
so we can avoid runtime imports in annotations, since we deferred their execution via from __future__ import annotations
. So with this example:
import stdnum._typing as t
x: t.Any
What happens is that, __annotations__
becomes {'x': 't.Any'}
, so the code t.Any
doesn't get executed until you (or a different dependency) runs typing.get_type_hints
or inspect.get_annotations
on your modules/functions which both use eval
internally to execute the stringized type expressions, so that's the first time stdnum._typing.__getattr__
gets called and the import happens.
A module-level __getattr__
to avoid the typing
import is admittedly less common, but there are some libraries that use that trick. What is a lot more common is the use of TYPE_CHECKING = False
and not supporting runtime introspection of type hints.
The more commonly used style would look like this:
TYPE_CHECKING = False
if TYPE_CHECKING:
from typing import Any
x: Any
In this case typing.get_type_hints
and inspect.get_annotations
would raise a NameError
, since Any
is never actually imported. Although PEP 649 will improve the situation for runtime introspection and add a mode where unresolvable names are replaced with typing.ForwardRef
, so you get back {'x': typing.ForwardRef('Any')}
instead of a NameError
for the above.
Hi @Daverball, I'm still working on merging this but I want to be really sure I fully understand how type checking works. I've already merged some parts of this in 6b9bbe2 and 8283dbb to make the remaining work easier to review. |
@arthurdejong Take all the time you need. I merged the current state of master and resolved the merge conflicts, so the diff should now reflect the current state of affairs. |
Closes #433
This adds inline type hints to the project. While trying to keep runtime impact to the absolute minimum, while still allowing for runtime introspection of type hints.
As a side effect this puts a hard limit of Python 3.7 as our minimum version, since older versions than 3.5 did not have support for any inline annotations at all and older versions than 3.7 did not have support for
from __future__ import annotations
. The experience of maintaining type hints is much worse without the latter, so it seems like a reasonable trade-off to me, given that 3.6 has been EOL since 2022.Otherwise it would make more sense to ship third party stubs.
Type hints are only guaranteed to be correct for Python 3.9+, which also seems like a reasonable trade-off, since current versions of type checkers don't support older versions than that anyways.
If you want to be able to introspect type hints,
typing_extensions
needs to be installed, which is once again reasonable, given that every major runtime typing library depends ontyping_extensions
and static type checkers have access to it via typeshed. Also for runtime introspection, the minimum Python version is 3.10, due to the use of|
for type unions, which also seems reasonable, given that people that make runtime use of annotations tend to use newer version of Python and that 3.9 will be EOL in October. But if this is unacceptable, we can replace the few places we use the operator withtyping.Union
andtyping.Optional
.This also fixes a couple of minor bugs that were discovered thanks to the static analysis provided by mypy.