-
Notifications
You must be signed in to change notification settings - Fork 5
feat: sails binary protocol program macro impl #1140
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
base: master-idl-v2
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @techraed, 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 introduces a significant overhaul to the binary protocol handling within Sails by implementing a new program macro. The core change shifts from byte-slice based message parsing to a structured 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 new binary protocol for Sails, moving from string-based routing to a more efficient and deterministic InterfaceId and entry_id system. Key changes include the addition of AnyServiceIds and ServiceMeta::BASE_SERVICES_IDS for compile-time service identification, and a significant refactoring of the program and service macros to support this new routing. The SailsMessage struct is introduced to encapsulate message headers and payloads, and new routing module provides const fn utilities for compile-time service counting and interface ID generation. The changes align well with the INTERFACE_ID_SPEC.md for deterministic ID generation, including lexicographical sorting of functions and base services.
The refactoring efforts, such as collecting data in a single pass to avoid borrow issues and applying modifications outside iteration loops, are good practices. The PR also addresses several items from its own TODO list, such as proper error handling (though some improvements can still be made), moving constants outside the WASM module, and cleaning up macro implementations.
| let is_async = <#service_type as #sails_path::gstd::services::Service>::Exposure::check_asyncness(interface_id, entry_id) | ||
| .unwrap_or_else(|| { | ||
| gstd::unknown_input_panic("Unknown call", &[]) | ||
| }); |
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.
The unwrap_or_else for check_asyncness currently provides a generic panic message. For better debugging and clarity, it would be more helpful to include the specific interface_id and entry_id that led to the "Unknown call" panic.
This would provide more context if a service call fails to be routed correctly.
let is_async = <#service_type as #sails_path::gstd::services::Service>::Exposure::check_asyncness(interface_id, entry_id)
.unwrap_or_else(|| {
gstd::unknown_input_panic(
&format!("Unknown call for interface_id: {:?}, entry_id: {}", interface_id, entry_id),
&[],
)
});|
|
||
| match route_id { | ||
| #(#route_dispatches)* | ||
| _ => gstd::unknown_input_panic("Unknown route_id", &[route_id]) |
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.
The unknown_input_panic for an Unknown route_id could be more informative. Including the interface_id along with the route_id in the panic message would provide additional context for debugging routing issues.
_ => gstd::unknown_input_panic(
&format!("Unknown route_id: {} for interface_id: {:?}", route_id, interface_id),
&[route_id],
)
TODO:
check_asyncnessto be method in concrete exposureconstsmap to be outside of wasm moduleNotice
The branch is created from
st-header-type-idlv2