-
Notifications
You must be signed in to change notification settings - Fork 64
Renderers py #651
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
Renderers py #651
Conversation
…ZeroableOption handling
|
sonicfromnewyoke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM 🔥
want a couple of things to change/discuss:
- modifications in the other packages and not only in the
renderers-py - e2e tests contain
Cargo.toml+Cargo.lockwhich shouldn't be here at all - e2e tests should generate (render) client +somehow validate that this code is working. Maybe we can use some kind of lining (like it was done with
cargo checkcommand)
|
I'll get to your PR as soon as I can but it's a heavy one to review and I've got lots of other things to prioritise at the moment so it might take a little while. Sorry in advance. 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review. This is looking promising. I've made a few high-level comments. I've not done any python for a while and never used the python SDKs for Solana, so I'd love for someone experiences with that world to give a deep review (@sonicfromnewyoke perhaps that's you?).
My overall feeling right now is that this is probably good enough but does not support the full type system that Codama describe. I can see this with the extension file that tries to patch specific use-cases. If that's the case, it's probably still okay to ship. The Rust renderer itself doesn't fully support the Codama type system because of the lack of a codec-like library (as Kit offers).
My most important question for whoever ends up shipping this Python render is: are you able to stick around once it's shipped to continue maintaining it?
No problem, it will be maintained continuously. |
|
Just to keep a record on GitHub about what has been discussed, the plan is now to have renderers has part of their own repositories, ideally owned by the team that maintains them. I have written a more detailed explanation here. Regarding the python renderer, I can see it has already been moved to its own repo available here meaning this PR can now be closed. Thank you so much for your work and I'll be sure to link this repository in the main README of the Codama repo. |
This package provides Python client renderers for Codama IDLs.
It is fully operational and compatible with the majority of Codama node types. Test cases align with those in renderers-js.
Sample demonstrations of generated effects are available at https://github.com/daog1/PyGenIDL001.