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 #17

Merged
merged 4 commits into from
Nov 8, 2024
Merged

Add type stubs #17

merged 4 commits into from
Nov 8, 2024

Conversation

pvcnt
Copy link
Contributor

@pvcnt pvcnt commented Oct 31, 2024

Those stubs are generated from the generated API documentation, with fixes provided in #15 and #16.

@pvcnt
Copy link
Contributor Author

pvcnt commented Oct 31, 2024

Also, would it be possible to create a new release with my recent changes? 🙏

@inducer
Copy link
Owner

inducer commented Oct 31, 2024

Is there a way to automate the generation, or at least have a CI job that checks that they're up to date?

@inducer
Copy link
Owner

inducer commented Oct 31, 2024

(And sure, I'd be happy to roll a release once we're done here.)

@pvcnt
Copy link
Contributor Author

pvcnt commented Oct 31, 2024

There seems to be a library to automatically generate those, but with limitations. I'm not familiar enough with Rust/Maturin to know how to wire it.

The following command can be used to validate the stubs:

python3 -m mypy.stubtest --ignore-missing-stub starlark

I need to wire it into the workflow.

@pvcnt
Copy link
Contributor Author

pvcnt commented Oct 31, 2024

I added a validation step. It's inside the "pytest" job, because I need the built module to be able to test stubs, and preferred not to create yet another job building the wheels.

Note: The --ignore-missing-stub is here because otherwise mypy complains about the starlark.starlark module that is not present in the stubs. That module is internal and should not be (maybe prefixing it with an underscore would be a good way to signal it btw, but that is another story).

@inducer
Copy link
Owner

inducer commented Nov 6, 2024

Thanks for adding the validation step. Upon playing around with it a bit, I noticed that passing --ignore-missing-stub will also ignore errors from stubs that are genuinely missing. (I tried by adding a dummy getter somewhere.) I tried to address this, but I've so far been unsuccessful.

@pvcnt
Copy link
Contributor Author

pvcnt commented Nov 8, 2024

I added another way to handle this via an allowlist. It specifies a list of modules where to ignore errors. It should ignore errors related to starlark.starlark, but not those related to starlark. Please note that this checker is by no means perfect, the docs mention a lot of limitations. It's really a best effort thing.

@inducer
Copy link
Owner

inducer commented Nov 8, 2024

Thanks! That seems to do the trick.

@inducer inducer merged commit dc54f42 into inducer:main Nov 8, 2024
11 checks passed
@pvcnt pvcnt deleted the patch-1 branch November 8, 2024 20:46
@inducer
Copy link
Owner

inducer commented Nov 8, 2024

A new release is on the way.

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