-
Notifications
You must be signed in to change notification settings - Fork 5
feat(idl-gen): type resolver, generic resolver, builder #1134
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
Conversation
Summary of ChangesHello @vobradovich, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the core IDL and client code generation mechanisms. The primary goal is to enhance the robustness and maintainability of the generation process by adopting a more structured builder pattern and compile-time templating. This change streamlines how IDL files are created and how corresponding client-side Rust code is generated, leading to cleaner codebases in projects that utilize this framework. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a major and impressive refactoring of the sails-idl-gen crate. It replaces the old Handlebars-based IDL generation with a more robust system based on scale-info reflection, using askama for templating. This is a significant architectural improvement. The introduction of TypeResolver, GenericResolver, and Builder structures makes the code more modular and maintainable.
My review has identified one critical issue regarding commented-out tests that must be addressed. I've also pointed out several medium-severity issues, including leftover debugging statements, the use of panic! and unimplemented! macros where a Result would be more appropriate, and some placeholder names and TODO comments that should be cleaned up for better maintainability.
Overall, this is a high-quality contribution, and once the critical issue is resolved, it will be a great addition to the codebase.
|
/gemini review |
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.
Code Review
This pull request introduces a major and impressive refactoring of the sails-idl-gen crate. The old handlebars-based implementation has been replaced with a more robust and type-safe solution using askama, syn, and a new builder pattern with a sophisticated type resolver. This is a significant improvement in terms of maintainability and capability.
My review focuses on a couple of areas for improvement:
- A potential bug in how service names are inferred from file paths in
sails-idl-gen. - Some confusing commented-out code in one of the examples.
Overall, this is excellent work. The new architecture for IDL generation is much more powerful and well-structured.
| // sails_rename::generate_idl_to_file::<ProxyProgram>( | ||
| // Some("ProxyProgram"), | ||
| // PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("proxy.idl"), | ||
| // ) | ||
| // .unwrap(); |
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.
techraed
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.
Awesome simplification of the idl-gen was done. That's notable especially when considering struct/enum fields type names resolution.
Some things to consider:
- I strongly recommend to move tests from #1101 PR from modules
meta2.rs,type_names.rs,idl-gen/tests/generator.rs,macros/tests. Passing these tests will fully assure that idl-gen works in accordance to the spec introduce in the mentioned PR - Interface ids are already included in the
master-idlv2. So checks for repeated definition of the base services done in the macro by comparing string names can be changed to generating const asserts comparingINTERFACE_IDs of base services. idl-genmust not generate the service several times. The check for duplicated interface id can be done inbuild_program_astfunction
No description provided.