-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Add stubs for pygit2 #11374
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
Add stubs for pygit2 #11374
Conversation
This comment has been minimized.
This comment has been minimized.
9e7fd82 to
7f3e7da
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
351dd7e to
37f496a
Compare
This comment has been minimized.
This comment has been minimized.
76491de to
87d8532
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
JelleZijlstra
left a comment
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.
Thank you. I looked through the code up to config.pyi, lightly comparing the stubs against the implementation. Mostly it looks good, but I see quite a few incorrect uses of AnyStr_co.
stubs/pygit2/pygit2/__init__.pyi
Outdated
|
|
||
| from . import enums | ||
| from ._build import __version__ as __version__ | ||
| from ._pygit2 import ( |
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.
The runtime just does from ._pygit2 import *, why not do that here?
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.
Originally I kept the star imports, but then mypy started to complain about wrong types of settings (it seems unable to distinguish the module settings and the global variable settings for example), and stubtest also wasn't happy with all those re-exported names either, so I had to spell out the names to basically help the checkers properly resolve the names.
stubs/pygit2/pygit2/__init__.pyi
Outdated
| get_credentials as get_credentials, | ||
| ) | ||
| from .config import Config as Config | ||
| from .credentials import ( |
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 one is also import *ed.
stubs/pygit2/pygit2/__init__.pyi
Outdated
| from .errors import Passthrough as Passthrough | ||
| from .filter import Filter as Filter | ||
| from .index import Index as Index, IndexEntry as IndexEntry | ||
| from .legacyenums import ( |
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.
And this one
stubs/pygit2/pygit2/__init__.pyi
Outdated
| LIBGIT2_VER: tuple[int, int, int] | ||
|
|
||
| def init_repository( | ||
| path: str | bytes | PathLike[AnyStr_co] | None, |
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.
A TypeVar doesn't make sense here since there is only one occurrence in the function. In any case, covariance is meaningless in a function.
You can instead use the StrOrBytesPath from _typeshed.
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.
Thanks for the suggestion; I only recently returned to Python development so I'm still getting familiar with the new typing scheme. I'll push an updated version later.
stubs/pygit2/pygit2/_build.pyi
Outdated
|
|
||
| __version__: str | ||
|
|
||
| def get_libgit2_paths() -> tuple[Path, dict[str, Any]]: ... |
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.
| def get_libgit2_paths() -> tuple[Path, dict[str, Any]]: ... | |
| def get_libgit2_paths() -> tuple[Path, dict[str, list[str]]]: ... |
stubs/pygit2/pygit2/callbacks.pyi
Outdated
| from .utils import _IntoStrArray | ||
|
|
||
| class Payload: | ||
| def __init__(self, **kw: dict[str, object]) -> None: ... |
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.
| def __init__(self, **kw: dict[str, object]) -> None: ... | |
| def __init__(self, **kw: object) -> None: ... |
Note that the type for **kwargs is the type of individual kwargs, not of the whole kwargs dict.
stubs/pygit2/pygit2/callbacks.pyi
Outdated
| directory: PathLike[AnyStr_co] | bytes | str | None = None, | ||
| paths: _IntoStrArray[AnyStr_co] = None, |
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.
It doesn't look like this function really needs to be generic. These types imply that directory and paths must be of the same type, which doesn't seem to be the case.
stubs/pygit2/pygit2/callbacks.pyi
Outdated
| callbacks: StashApplyCallbacks | None = None, | ||
| reinstate_index: bool = False, | ||
| strategy: CheckoutStrategy | None = None, | ||
| directory: PathLike[AnyStr_co] | bytes | str | None = None, |
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.
Same here
stubs/pygit2/pygit2/config.pyi
Outdated
|
|
||
| from _cffi_backend import _CDataBase | ||
|
|
||
| def str_to_bytes(value: object, name: str) -> bytes: ... |
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.
| def str_to_bytes(value: object, name: str) -> bytes: ... | |
| def str_to_bytes(value: str, name: object) -> bytes: ... |
It throws a TypeError if it's not a str
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.
That must be a thinko. Fixed in the next push.
stubs/pygit2/pygit2/config.pyi
Outdated
| def __del__(self) -> None: ... | ||
| def __contains__(self, key: str) -> bool: ... | ||
| def __getitem__(self, key: str) -> str: ... | ||
| def __setitem__(self, key: str, value: bool | int | _CDataBase | PathLike[AnyStr_co] | bytes | str | None) -> None: ... |
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.
Again, don't think AnyStr_co makes sense here.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
I've just removed all |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
It seems mypy cannot handle the re-import of |
This is the same as python/typeshed#11374, and allows proper type-checking before it is accepted upstream.
The upstream library is very tricky to type (likely requires nontrivial refactoring), and only contains partial type information, but stubs are a lot easier because only the public signatures are involved this way, so I plan to first make the library usable in typed projects by making stubs available here, then gradually work my way upstream. The stubs are auto-generated then completed with fully manual inspection of every Python source file. The `_pygit2.pyi` comes from upstream and is mostly untouched except for required style changes, the signature of `options()`, and `FilterSource` which is missing from upstream.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
|
It seems simply ignoring the type check error on the |
This is the same as python/typeshed#11374, and allows proper type-checking before it is accepted upstream.
The upstream library is very tricky to type (likely requires nontrivial refactoring), and only contains partial type information, but stubs are a lot easier because only the public signatures are involved this way, so I plan to first make the library usable in typed projects by making stubs available here, then gradually work my way upstream.
The stubs are auto-generated then completed with fully manual inspection of every Python source file. The
_pygit2.pyicomes from upstream and is mostly untouched except for required style changes, the signature ofoptions(), andFilterSourcewhich is missing from upstream.