libgit2 / pygit2

Python bindings for libgit2
https://www.pygit2.org/
Other
1.58k stars 382 forks source link

Type hints for _pygit2 (C module) #1121

Closed jorio closed 2 years ago

jorio commented 2 years ago

In IDEs like PyCharm, type hinting doesn't work well with the contents of pygit2._pygit2 (the part of pygit2 that's written in C). This hinders autocompletion for important classes like Commit, Branch, etc.

This PR introduces _pygit2.pyi, which annotates types implemented in C. This makes it much more pleasant to work with pygit2 in an IDE.

In a nutshell, I used stubgen to create type stubs. Then I refined them manually, and I ensured that stubtest doesn't report errors.

If we merge this PR, we should keep the stubs file in sync with any API changes we make in the C codebase from now on. The consistency of the type stubs can be tested with build.sh stubtest — I suggest we add this step to CI. Stubtest isn't perfect: it won't catch discrepancies in argument types, but it will raise errors if we miss a new function or member variable.


This PR ties in with issue #709, but it doesn't fix it completely: we'll still need to annotate more places in the Python codebase.

jdavid commented 2 years ago

This is great!

Just sometimes we loss some information, for example:

-  "applies(diff, location=GIT_APPLY_LOCATION_INDEX) -> bool\n"
+  "applies(diff: Diff, location: int = ...) -> bool\n"

Here we win the type information, in the docs for example this renders a nice link for Diff. But we lose the default value for the location argument. Cannot we keep both?

jorio commented 2 years ago

Fixed! That was an oversight.

jdavid commented 2 years ago

For discover_repository today the docs are (see https://www.pygit2.org/repository.html#pygit2.discover_repository):

pygit2.discover_repository(path[, across_fs[, ceiling_dirs]]) → str

With this PR it will read:

pygit2.discover_repository(path: str, across_fs: bool = False, ceiling_dirs: str = ...) → str

The ellipsis default value for ceiling_dirs is kind of undefined. In this particular case the default value should be the empty string; actually omitting ceiling_dirs will pass NULL to libgit2, but in this case (git_repository_discover) it's equivalent to the empty string.

From what I've seen the documentation is generated from the doc strings, while the editors use the information in the type hints?

So both have to be kept synchronized. For example Worktree.prune(force=False) is correctly rendered in the documentation, even though in the stubs file it uses the ellipsis.

Then, can you please:

Thanks!

jorio commented 2 years ago

Some ellipses were oversights (such as Worktree.prune), but most were deliberate, because many pygit2 functions implemented in C don't have fallback values for optional parameters. For instance:

Tree.diff_to_tree([tree: Tree, flags: int, context_lines: int, interhunk_lines: int, swap: bool = False])

When calling this function, it's OK to omit tree. However, passing tree=None causes TypeError, which isn't the same effect as omitting the argument. This is because the C implementation doesn't check for None; it only considers whether the parameter is present or not.

This construct (optional arguments without a default value) is difficult to translate to the type stub syntax. It'd be misleading to have tree=None, and the [] syntax doesn't exist in type stubs, which is why I went for tree=....

That said, I understand your concern about losing information in the docs, so my latest commit in the PR restores the old square-bracket notation in the PyDoc_STRVARs wherever there is no adequate default value.

The type stubs still have some ellipses out of necessity, but this is no problem for the HTML docs because Sphinx only looks at the C code, not at the stubs.