-
Notifications
You must be signed in to change notification settings - Fork 177
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
proc-macros: rename parameter names #1365
Conversation
Looks overall good, nice job I'm not following why you need to rename parameters when those passed as Array (these are omitted by the client)? In such scenarios then variable doesn't correspond to the actual param name, so why is it needed? The contrary applies to JSON Objects where the key is the same name passed in the call with some glue to convert snake_case to camelCase. Then I follow why these need to be renamed to support reserved rust keywords etc.... I'll take a closer look tomorrow. |
You are absolutely right, there is no point (actually no way) to rename parameters in arrays. |
Co-authored-by: Niklas Adolfsson <[email protected]>
- expose potentially renamed name as String insead of Option in RpcFnArg::name() - make RpcFnArg::rename_to private to migrate to new RpcFnArg::name() - adapt render_client.rs and render_server.rs to changes
Looks great, I'll can push a fix to unblock the UI tests tomorrow it's probably the latest rustc release that broke them. |
- remove unnecessary param_kind: map - add array params test to prevent accidental breakage through param renaming
alias_vals | ||
.push_str(&format!(r#"alias = "{}""#, heck::ToLowerCamelCase::to_lower_camel_case(name.as_str()))); | ||
|
||
let serde_rename = quote!(#[serde(rename = #name)]); |
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.
This is needed because arg_pat below can't be reserved rust names such as type
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.
Nice work looks good
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.
LGTM, just a couple of nits!
Co-authored-by: James Wilson <[email protected]>
Decoupling rust parameter names from (de)serialized json is essential for me and useful in general.
Some usecases include:
type
(currently impossible)Those are both usecases I personally have.
The PR implements this by introducing a new attribute for the
rpc
macro calledargument
.I previously responded to an outdated similar PR here.
Minimal working example:
I divided the changes into multiple commits because they are a bit lengthy.
This is my first open source contribution, I am eager to learn, let me know what can be improved.