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

Adds stub type definitions to surgepy. Fixes #7362 #7363

Merged
merged 3 commits into from
Dec 6, 2023

Conversation

KingaJanicka
Copy link
Contributor

I've manually ran pybind11-stubgen surgepy and added the resulting stubs to the surgepy package install routine. This is similar to how pybind11-stubgen author recommends these stubs be used; see sizmailov/pybind11-stubgen#104 (comment).

It's possible there is a better way of accomplishing this, however I am very unfamiliar with skbuild.

@narenratan
Copy link
Contributor

Hi there, Naren here (person who set up the surgepy packaging). This looks great to me! I can successfully pip install surgepy with this branch and the surgepy tests pass.

@baconpaul
Copy link
Collaborator

baconpaul commented Dec 4, 2023

Yes happy to merge

My only question is what do we do if we update the bindings? Can we add that as a comment in the cpp file so we can keep things in synch?

@KingaJanicka
Copy link
Contributor Author

@baconpaul that sounds like a good idea! I'll leave it to your discretion where to put that. Ideally it would generate the stubs post install but I'm not sure how to do that with skbuild (pybind11-stubgen expects the package to be installed via pip afaik).

@baconpaul
Copy link
Collaborator

Really just put a block comment in this file after the boilerplate comme t with what you ran
https://github.com/surge-synthesizer/surge/blob/main/src/surge-python/surgepy.cpp

And add yourself to authors and push and we can merge

Cool thanks!

Added comment explaining how to generate stub types.
@KingaJanicka
Copy link
Contributor Author

@baconpaul have done both, thank you!

@baconpaul
Copy link
Collaborator

Awesome one of us can fix up that clang format and merge this for 1.3 this eeek

@mkruselj
Copy link
Collaborator

mkruselj commented Dec 6, 2023

@KingaJanicka You haven't added yourself to AUTHORS file. Please do that as well before we merge. :) Thanks!

@baconpaul
Copy link
Collaborator

@KingaJanicka You haven't added yourself to AUTHORS file. Please do that as well before we merge. :) Thanks!

There’s an authors diff in this pr! Did you not read the merged diff maybe?

I will format and merge this this am

@baconpaul baconpaul merged commit 3a18c1d into surge-synthesizer:main Dec 6, 2023
10 checks passed
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.

4 participants