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

Add type stubs & minimal validation #110

Merged
merged 2 commits into from
Jan 28, 2024

Conversation

PeterJCLaw
Copy link
Contributor

This adds type stubs to the published package, so that consumers can benefit from type checking despite the Cython implementation of this package. The stubs are manually constructed as there does not appear to be a mature solution which produces good stubs from Cython code. (It is actually unclear if such a solution is possible without annotation of the Cython source)

Validation of the stubs is achieved by annotating the tests and relying on the existing test coverage to explore all relevant members & operations. A new tox environment runs mypy against the tests.

Fixes #105

Builds on #109.

@Ezibenroc
Copy link
Owner

Great work!

The stubs are manually constructed as there does not appear to be a mature solution which produces good stubs from Cython code.

That's kind of crazy that there is nothing to automate this stub creation.

@PeterJCLaw
Copy link
Contributor Author

That's kind of crazy that there is nothing to automate this stub creation.

There's work ongoing it seems, but there didn't seem to be a default just-works solution. I didn't look too much into all the options, partly as some of them looked like they'd be fairly disruptive. https://github.com/mike-huls/cythonbuilder for example seems to be mentioned as being able to do it, but also seems to be a separate build process -- which I didn't want to force on the project just for annotations. I didn't look at all into how it actually works though, nor whether it can be used just for the stubs.

@Ezibenroc
Copy link
Owner

Alright!
The PR is still marked as "work-in-progress", do you still have changes to do before I review?

I am working on #106, adding support to 64-bit bitmaps. I feel it might be easier to first merge #110 and then #106, rather than the other way around. What do you think?

@PeterJCLaw
Copy link
Contributor Author

It's mostly that I'll rebase this on master once #109 is merged, so that the diff is clearer. I also wanted to self-review the types added to the tests.

I haven't looked at #106 in detail, though it seems that that adds quite a lot of stuff? If so, yeah it's probably simpler to get this PR in first. Hopefully extending the changes here to cover that will be fairly straightforward.

@Ezibenroc
Copy link
Owner

I just merged #109, tell me when you are ready for review 👍

This adds type stubs to the published package, so that consumers
can benefit from type checking despite the Cython implementation
of this package. The stubs are manually constructed as there does
not appear to be a mature solution which produces good stubs from
Cython code. (It is actually unclear if such a solution is
possible without annotation of the Cython source)

Validation of the stubs is achieved by annotating the tests and
relying on the existing test coverage to explore all relevant
members & operations. A new tox environment runs mypy against the
tests.

Fixes Ezibenroc#105
Mypy complains that this doesn't have a self/cls argument, however
adding @staticmethod causes test failures on Python < 3.10 as the
resulting object isn't callable (presumably because the class hasn't
finished initiating when we call the method). Hoisting this to a
function avoids the issue entirely, at the cost of this helper
being slightly further from its usages.
@PeterJCLaw
Copy link
Contributor Author

For clarity: this is ready for review now.

max_value: int
min_value: int
sum_value: int
cardinality: int
Copy link
Owner

Choose a reason for hiding this comment

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

That made me think, it could be nicer to replace this dict by a proper dataclass. But this is out of scope of this PR of course.

@Ezibenroc Ezibenroc merged commit 4f86dc3 into Ezibenroc:master Jan 28, 2024
18 checks passed
@PeterJCLaw PeterJCLaw deleted the type-stubs branch January 28, 2024 22:13
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.

Publish type annotations
2 participants