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.

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

Type stubs #4

Open
davetapley opened this issue Sep 28, 2023 · 10 comments · May be fixed by #6
Open

Type stubs #4

davetapley opened this issue Sep 28, 2023 · 10 comments · May be fixed by #6

Comments

@davetapley
Copy link

FYI I've put some stubs up here: python/typeshed#10796

But maybe want them in this repo?

@mina86
Copy link
Owner

mina86 commented Sep 28, 2023

Indeed, would be great to have types in the code.

@davetapley
Copy link
Author

davetapley commented Sep 28, 2023

@mina86 so I think we can either put them inline (which would mean making the library require Python >3.5), or add a py.typed file along with the .pyi per: https://peps.python.org/pep-0561/#packaging-type-information

Any preference?

@mina86
Copy link
Owner

mina86 commented Sep 28, 2023

I say inline. With pygtrie I’m somewhat conservative with required Python version and with 3.8 still getting security updates I’d like to keep the version ≤ 3.8.

@mina86
Copy link
Owner

mina86 commented Sep 29, 2023

FYI, just pushed c0dd534 which removes 2.x compatibility. (Honestly I thought it was already upstream).

@Avasam
Copy link

Avasam commented Sep 29, 2023

Just FYI, you can have inline type annotations for Python <3.5 using type comments.

You can also include and ship stubs directly with the library if that becomes too unwieldy.

And finally there's many ways to support more modern types and annotations for older python syntax (< 3.10):

  • string annotations
  • typing.TYPE_CHECKING block and type aliases
  • base type aliases (typing.List, typing.Dict, etc.)
  • from future import __annotations__

You would also still need to add a py.typed file with inline annotations for type-checkers to consider the package as "typed".

@mina86
Copy link
Owner

mina86 commented Sep 29, 2023

Just FYI, you can have inline type annotations for Python <3.5 using type comments.

It’s probably not worth the effort though. Using comments is considerably less ergonomic.

You can also include and ship stubs directly with the library if that becomes too unwieldy.

I’d rather have it all in the source code because that makes maintenance easier. Less chance to forget to update types when editing source code. It also aids reading the code since type hints serve as documentation.

And finally there's many ways to support more modern types and annotations for older python syntax (< 3.10):

Yes. In general my attitude is that the code should execute in as old version as Python as is not too burdensome to support but it’s fine if latest Python version is required to get the most benefits of the types. I’m also not concerned with runtime availability of the type annotations (unless someone comes and opens an Issue for that; then I’ll think about it).

@davetapley
Copy link
Author

Great! I'll open a PR with the types I added for my purposes, then I'll leave it up to you whether you want to merge as is or collaborate on a separate branch to add types for everything.

@Avasam I think these are all okay with 3.5, right? Feedback also welcome :)

from typing import Callable, Generic, Iterable, TypeVar

C = TypeVar('C')
V = TypeVar('V')
Children = Iterable[C]
Path = tuple[str, ...]
PathConv = Callable[[Path], str]
NodeFactory = Callable[[PathConv, Path, Children[C], V], C]


class StringTrie(Generic[V]):
    def __init__(self, separator: str | None = None) -> None: ...
    def __setitem__(self, key: str, value: V) -> None: ...
    def __getitem__(self, key: str | slice) -> V: ...

    def keys(self, prefix: str | None = None,
             shallow: bool = False) -> list[str]: ...

    def values(self, prefix: str | None = None,
               shallow: bool = False) -> list[V]: ...

    def items(self, prefix: str | None = None,
              shallow: bool = False) -> list[tuple[str, V]]: ...

    def traverse(self, node_factory: NodeFactory[C, V]) -> C: ...

    def enable_sorting(self, enable: bool) -> None: ...

@mina86
Copy link
Owner

mina86 commented Oct 2, 2023

Path = tuple[str, ...]

I think that’s 3.9 syntax or something. This should be typing.Sequence[str] I think.

str | None

And that’s Python 3.10. For T | None this needs to be typing.Optional[T].

def __init__(self, separator: str | None = None) -> None: ...

The default value for the separator is / so this isn’t quite right. Also, in code this is __init__(self, *args, **kw) so there’s some more typing necessary here to get it in code.

@Avasam
Copy link

Avasam commented Oct 2, 2023

I can never remember by hearth past Python 3.7 . The best way is to actually try it.

Some things I notice immediately: collections types are not subscriptable in Python 3.8, you'll have to import from typing.
ie: typing.Tuple[str] instead of tuple[str].

The union operator (|) is Python 3.10 syntactic sugar syntax for typing.Union.

As long as Python 3.6 is supported in the project, you can't even use from __future__ import annotations to backport annotations syntax (it wouldn't work for type aliases anyway, unless you put them behind TYPE_CHECKING checks).

You can work around some annotation issues with stringified annotations (ie: foo: "list[int]" = []), but may as well stay consistent and not use it when you don't need it.

(all comments above only apply for inline types, stub files wouldn't matter)

You could also forego some type aliases, especially if they risk being confused with other types (ie: pathlib.Path).


I just noticed this project is a single file-module. you'll have to make it a package (pygtrie/__init__.py) instead to be able to ship a py.typed marker.


You can (should) automate testing this. The best thing to do at the very least is to have some smoke-screen tests for all module that do nothing but import. To ensure no import or syntax error on Python 3.5.
Since this project is a single module, and already has tests, that part is already done as long as the CI validates it.

Tooling-wise you'd have to use outdated versions to check all the way down to 3.5 .

I'd still at least recommend testing with type-checkers, to make sure your annotations work for your users:

  • pyright (should support all versions)
  • mypy<1.5 for Python 3.7
  • pytype and pyre would be the next popular ones one the list (typeshed ensures tests for pytype), but their support is a lot more limited

And optionally type/syntax linters like:

  • Flake8-PYI (maybe, might not work for this project for 3.5/.36, it's more intended at type-stubs)
  • flake8-type-checking (Validates python >= 3.7 only, but should still work for older)
  • flake8-annotations (check for missing annotations, although both mypy and pyright can do that, I'd recommend using those instead)
  • pyupgrade<3.0 for Python<3.6 support, helps you update old code from Python versions you no longer support
  • flake8-future-annotations (not usable until Python >=3.7)

All of the linters mentioned above have been re-implemented in Ruff, which I'd highly suggest instead (simpler, faster, and you can also disable rules that try to enforce a syntax too modern, unlike pyupgrade)

Even if you still need to manually validate for 3.5-3.6, you can at least automatically ensure and enforce that you'll write code fully compatible with >=3.7


If you have typing questions, feel free to ask over at their discussion forum or try the typing chat room on gitter.im (links taken from https://github.com/python/typeshed#discussion)

@Avasam
Copy link

Avasam commented Oct 2, 2023

I double checked, Callable and Iterable should both be accessible from collections.abc in 3.5 and so should be imported from there instead of using the alias defined in typing.
PEP 585 syntax. This is a Python 3.9 deprecation. Checked by Flake8-PYI rule Y022


(optional as well) You should also prefer explicitly marking your type aliases as such. Checked by Flake8-PYI rule Y026. But on Python <3.10 that will require a dependency on typing-extensions.TypeAlias, and wrapping it behind TYPE_CHECKING if you want to avoid runtime dependency on typing-extensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants