Skip to content

Conversation

@xen0n
Copy link
Contributor

@xen0n xen0n commented Feb 7, 2024

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.

@github-actions

This comment has been minimized.

@xen0n xen0n force-pushed the pygit2 branch 4 times, most recently from 9e7fd82 to 7f3e7da Compare February 7, 2024 10:46
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@xen0n xen0n force-pushed the pygit2 branch 2 times, most recently from 351dd7e to 37f496a Compare February 7, 2024 11:36
@github-actions

This comment has been minimized.

@xen0n xen0n force-pushed the pygit2 branch 2 times, most recently from 76491de to 87d8532 Compare February 7, 2024 11:51
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@JelleZijlstra JelleZijlstra left a 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.


from . import enums
from ._build import __version__ as __version__
from ._pygit2 import (
Copy link
Member

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?

Copy link
Contributor Author

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.

get_credentials as get_credentials,
)
from .config import Config as Config
from .credentials import (
Copy link
Member

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.

from .errors import Passthrough as Passthrough
from .filter import Filter as Filter
from .index import Index as Index, IndexEntry as IndexEntry
from .legacyenums import (
Copy link
Member

Choose a reason for hiding this comment

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

And this one

LIBGIT2_VER: tuple[int, int, int]

def init_repository(
path: str | bytes | PathLike[AnyStr_co] | None,
Copy link
Member

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.

Copy link
Contributor Author

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.


__version__: str

def get_libgit2_paths() -> tuple[Path, dict[str, Any]]: ...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def get_libgit2_paths() -> tuple[Path, dict[str, Any]]: ...
def get_libgit2_paths() -> tuple[Path, dict[str, list[str]]]: ...

from .utils import _IntoStrArray

class Payload:
def __init__(self, **kw: dict[str, object]) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Comment on lines 70 to 71
directory: PathLike[AnyStr_co] | bytes | str | None = None,
paths: _IntoStrArray[AnyStr_co] = None,
Copy link
Member

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.

callbacks: StashApplyCallbacks | None = None,
reinstate_index: bool = False,
strategy: CheckoutStrategy | None = None,
directory: PathLike[AnyStr_co] | bytes | str | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

Same here


from _cffi_backend import _CDataBase

def str_to_bytes(value: object, name: str) -> bytes: ...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Contributor Author

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.

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: ...
Copy link
Member

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.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@xen0n
Copy link
Contributor Author

xen0n commented Feb 28, 2024

I've just removed all AnyStr_co occurrences and fixed the rest of signature mistakes pointed out in the review comments.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@xen0n
Copy link
Contributor Author

xen0n commented Feb 28, 2024

It seems mypy cannot handle the re-import of Repository with different types if the star-imports have to be preserved in the stubs:

stubs/pygit2/pygit2/__init__.pyi:24: error: Incompatible import of "Repository" (imported name has type "type[pygit2.repository.Repository]", local name has type "type[pygit2._pygit2.Repository]")  [assignment]

xen0n added a commit to xen0n/ruyi that referenced this pull request Feb 28, 2024
This is the same as python/typeshed#11374, and allows proper
type-checking before it is accepted upstream.
xen0n added 3 commits March 2, 2024 15:34
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.
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2024

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@xen0n
Copy link
Contributor Author

xen0n commented Mar 2, 2024

It seems simply ignoring the type check error on the Repository re-import doesn't work. When I was working on a project with the content of this PR integrated into its stubs/ I see Pylance failing to infer type for pygit2.Repository (saying the name doesn't exist).

xen0n added a commit to xen0n/ruyi that referenced this pull request Mar 4, 2024
This is the same as python/typeshed#11374, and allows proper
type-checking before it is accepted upstream.
@JelleZijlstra JelleZijlstra merged commit 9a4b605 into python:main Mar 12, 2024
@xen0n xen0n deleted the pygit2 branch March 12, 2024 07:28
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 this pull request may close these issues.

2 participants