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

Export type annotations for public use #95

Merged
merged 17 commits into from
Dec 14, 2023
Merged

Export type annotations for public use #95

merged 17 commits into from
Dec 14, 2023

Conversation

bgilbert
Copy link
Contributor

Type annotations are currently internal to pyfuse3, so applications can't use them for their own type checking. Fix some bugs and omissions in the annotations and make them public by adding a py.typed file.

To do so, we're required to move our modules into a Python package. Do this, but add forwarders for the _pyfuse3 and pyfuse3_asyncio modules to avoid breaking compatibility.

Probably best reviewed as individual commits. GitHub's rendering of the overall diff is somewhat unhelpful.

Preparatory for next commit.
If we want to export our type information, we're required to wrap our
modules in a package.  Do so, but leave compatbility wrappers in the
old locations.
Copy link
Collaborator

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Had a quick look, lgtm - but I am not that familiar with type annotations and cython.

@Nikratio can you have a look?

src/pyfuse3/_pyfuse3.py Outdated Show resolved Hide resolved
src/pyfuse3/__init__.pyi Outdated Show resolved Hide resolved
@bgilbert bgilbert marked this pull request as draft November 22, 2023 15:16
@bgilbert
Copy link
Contributor Author

bgilbert commented Nov 22, 2023

Thanks for the quick review!

Marked as draft; I have a few more changes coming.

Use string annotations to work around unparameterizable asyncio types
in Python 3.8.

Also replace one use of set[] with Set[], again for Python 3.8.
__cinit__() is an implementation detail; __init__() is what callers see.
We were also importing async_wrapper without referencing or re-exporting
it.  It's not clear that it's useful outside the pyfuse3 implementation,
so for now, drop the import rather than re-exporting.
@bgilbert bgilbert changed the title RFC: Export type annotations for public use Export type annotations for public use Nov 22, 2023
@bgilbert
Copy link
Contributor Author

Okay, this should be ready for review. CI should pass now. This update fixes some additional type issues, adds a new XAttrNameT for xattr names (subclassing bytes), and adds a mypy run to CI.

I've gone through pyfuse3-stubs and compared its types to the ones in pyfuse3, incorporating any fixes I found. pyfuse3-stubs includes additional types for some pyfuse3 internals like g and _NANOS_PER_SEC, which are omitted here.

cc:

  • @odgalvin — original contributor of pyfuse3 types
  • @rhelmot — author of pyfuse3-stubs, which I think is superseded by this PR
  • @Nikratio — cc'd in an edit of a comment above, which IIRC doesn't generate a notification

@bgilbert bgilbert marked this pull request as ready for review November 23, 2023 00:04
The application may want to use them at runtime, e.g. with typing.cast().
This tells the typechecker to complain if the application tries to
assign to these attributes.
It makes the logs a bit easier to read.  Preparatory for next commit.

Drop redundant set -e; GitHub Actions configures it automatically.
Sphinx autoclass is currently confused by NewTypes, so document those by
hand.

Also, Sphinx wants to document the NewTypes as coming from
pyfuse3._pyfuse3, and there doesn't seem to be a setting or rST directive
to convince it otherwise.  Work around this by mangling the types'
__module__ attributes from conf.py.
@bgilbert
Copy link
Contributor Author

Updated once again, to fix docs and to re-export the NewTypes at runtime for use e.g. with typing.cast().

@bgilbert
Copy link
Contributor Author

@Nikratio, per #95 (review), is there any chance you could take a look?

@Nikratio
Copy link
Contributor

Sorry for the delay. It seems that src/pyfuse3/_pyfuse3.py is not recognized as a rename but instead a completely new file, which makes the diff very large. Do you know why this might be the case? Did you perhaps do large whitespace changes by accident?

@bgilbert
Copy link
Contributor Author

It's probably because there's still a src/_pyfuse3.py, it's just a stub now. I'd recommend reviewing commit-by-commit. I tried to keep each commit legible and self-contained.

@Nikratio Nikratio merged commit 9cd9699 into libfuse:master Dec 14, 2023
10 checks passed
@bgilbert bgilbert deleted the typing branch December 14, 2023 09:23
@bgilbert
Copy link
Contributor Author

Thanks for the reviews!

@bgilbert
Copy link
Contributor Author

Do you have a sense of when this might end up in a release?

@ThomasWaldmann
Copy link
Collaborator

@bgilbert there is only 1 small issue left to do in the 3.3.1 milestone, so guess I could do a release in the near future.

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.

3 participants