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 API, type hints for existing bindings. #168

Conversation

MarquessV
Copy link
Contributor

@MarquessV MarquessV commented Mar 14, 2023

This is a pretty large PR, but the approach was very mechanical.

For any existing module in quil-py:

  1. I checked that it mirrored quil-rs, and if not, reconciled them to match. In most cases, this was a matter of refactoring quil-rs instructions out of mod.rs and into a submodule. There were a few cases where quil-py had a module in the wrong place. For example, the expression module was in instructions instead of it's own top-level module. In these cases, I refactored quil-py to match.

  2. Made sure each binding had a new constructor, derived the same attributes as its corresponding quil-rs type, and wasn't missing any derived implementations (e.g. impl_hash! for Hash or fn __richcmp__ for PartialEq/Eq).

  3. Added missing variants to the PyInstruction enum, if the module contained any of the needed types.

  4. Made sure they all had type hints and didn't fail any stubtest checks. As part of this I had to flatten the type stubs. We can't break them into private modules for organization because those private modules aren't actually exported by the package. Technically, there are still two stubtest failures. The bindings export quil.quil and quil.validation.quil.validation.identifier as empty modules. I've seen this in the qcs_sdk as well, and I think it's a bug with either the pyo3 macros or pyo3 itself. We could silence the errors by adding dummy stubs, but I'd rather have the errors and not expose them to users. I created this issue to follow-up.

I also handled a few extra things, like building out the Expression bindings, because they were in the path and it made sense to just get it out of the way here.

Closes #167, and because the frame instruction bindings were pre-existing, this also closes #162. This also adds a Makefile.toml with a workflow for running stubtest to validate the type stubs.

@MarquessV MarquessV marked this pull request as ready for review March 15, 2023 18:12
quil-py/Makefile.toml Show resolved Hide resolved
@MarquessV MarquessV merged commit da9cfb3 into 1455-python-support-for-program-and-beyond Mar 16, 2023
@MarquessV MarquessV deleted the 167-add-instruction-api-type-hints-for-existing-bindings branch March 16, 2023 23:45
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