Proc mac support map param #544

DefinitelyNotHilbert · 2021-10-29T21:04:16Z

This is my MR for #538.

Added param_format parameter to the method and subscription attributes. It takes a string parameter, "map"to make the macro generate a function that sends the parameters as a map, everything else will fall back to the default "array". I'm not too happy with the fact that it is a string, so if anybody has an alternative, be my friend :)
Updated render_method and render_sub to generate the necessary code. There's an additional function that extracts the parameter names from the function signature

I wasn't sure how / if you test your macros, so I didn't do anything related to unit test, but I can confirm that the code is generated correctly and works, judging from how it interacts with json_rpc APIs.

Looking forward to your comments :)

polkadot address: 13bL3pENnNbFKYD6GPjibJHv4jUTnwNPoVjp9YpU4oJfFefS

niklasad1

great,

please add a test in jsonrpsee/proc-macros/tests/ui/correct/* for this, to test that it actually works with the servers too.

you can look in jsonrpsee/proc-macros/tests/ui/correct/basic how to do it so up to you whether to add a new file for this or in existing one...

proc-macros/src/rpc_macro.rs

@niklasad1

- Addressed @niklasad1's suggestion to use an Option instead of just defaulting to "array".

proc-macros/src/rpc_macro.rs

- Use enum for param format. - Extract parsing logic into separate function. - Add ui test.

DefinitelyNotHilbert · 2021-10-31T12:18:50Z

great,

please add a test in jsonrpsee/proc-macros/tests/ui/correct/* for this, to test that it actually works with the servers too.

you can look in jsonrpsee/proc-macros/tests/ui/correct/basic how to do it so up to you whether to add a new file for this or in existing one...

I've added a test case in the spirit of basic.rs but I'm not 100% that it's exactly what you want.

niklasad1

nice work, thank you

LGTM

niklasad1 · 2021-10-31T16:08:14Z

@DefinitelyNotHilbert just run cargo fmt --all and push it then the CI should be happy

dvdplm

Looks great, tyvm.

Some docs here about the new attr would be good.

proc-macros/tests/ui/correct/param_format.rs

proc-macros/src/render_client.rs

proc-macros/src/attributes.rs

proc-macros/src/render_client.rs

maciejhirsz

I'm going to be a bit more pedantic here, sorry :D. Added suggestions on how to turn the "array"|"map" into array|map and one other nit.

In general I think that since this is a binary option, and it's always going to be a binary option, we should probably use a flag argument. So instead of:

#[method(name = "foo", param_kind = map)]

You'd just write:

#[method(name = "foo", serialize_as_map)]

Or some such. I reckon that's easier to grok since you don't have to look up possible values, and code should also be easier since there are fewer error conditions.

maciejhirsz · 2021-11-03T09:25:36Z

proc-macros/src/attributes.rs

+pub(crate) fn parse_param_kind(arg: Result<Argument, MissingArgument>) -> ParamKind {
+	match optional(arg, Argument::string).unwrap().as_deref() {
+		None | Some("array") => ParamKind::Array,
+		Some("map") => ParamKind::Map,
+		err => panic!("param_kind must be either `map` or `array`, got {:?}", err),
+	}
+}


We should return an error here, unwrap is going to throw away span information on invalid parse of the argument value. Instead of a string we could parse to raw Argument::value using syn::Ident so you can skip quotes, plus then you have the span to use to emit an error instead of a panic.

No guarantees that this compiles:

Suggested change

pub(crate) fn parse_param_kind(arg: Result<Argument, MissingArgument>) -> ParamKind {

match optional(arg, Argument::string).unwrap().as_deref() {

None | Some("array") => ParamKind::Array,

Some("map") => ParamKind::Map,

err => panic!("param_kind must be either `map` or `array`, got {:?}", err),

}

}

pub(crate) fn parse_param_kind(arg: Result<Argument, MissingArgument>) -> syn::Result<ParamKind> {

let kind: Option<syn::Ident> = optional(arg, Argument::value)?;

match kind {

None => Ok(ParamKind::Array),

Some(ident) if ident == "array" => Ok(ParamKind::Array),

Some(ident) if ident == "map" => Ok(ParamKind::Map),

Some(ident) => Err(Error::new(ident.span(), "param_kind must be either `map` or `array`")),

}

}

I figure that this won't be necessary if we go with your suggestion of turning parma_kind into a flag, because we can get rid of the function.

Personally, I prefer the more verbose version (that's why I've implemented it in this way), but I completely get your point. Since this is obviously not my library, I'm happy to turn it into a flag if you want, I just care about the feature as a whole.

Yeah, this is only if we want to keep the verbose variant.

I don't have a super strong opinion on this, and don't necessarily want to turn it into a bike shed. We can still change the syntax of it later before 0.5 is stabilized.

proc-macros/src/render_client.rs

proc-macros/tests/ui/correct/param_kind.rs

@maciejhirsz

Apply @maciejhirsz formatting suggestion. Co-authored-by: Maciej Hirsz <1096222+maciejhirsz@users.noreply.github.com>

- make parameter encoding DRY - remove strings from param_kind - return result from parse_param_kind

maciejhirsz

@DefinitelyNotHilbert cheers!

dvdplm · 2021-11-03T14:58:06Z

/tip medium

substrate-tip-bot · 2021-11-03T14:58:07Z

Please fix the following problems before calling the tip bot again:

Contributor did not properly post their Polkadot or Kusama address. Make sure the pull request has: "{network} address: {address}".

dvdplm · 2021-11-03T14:59:05Z

@DefinitelyNotHilbert Thank you for your work here. Please post a comment with a polkadot address to receive your tip. :)

DefinitelyNotHilbert · 2021-11-04T21:58:46Z

polkadot address: 13bL3pENnNbFKYD6GPjibJHv4jUTnwNPoVjp9YpU4oJfFefS

dvdplm · 2021-11-04T22:04:16Z

/tip medium

substrate-tip-bot · 2021-11-04T22:04:17Z

Please fix the following problems before calling the tip bot again:

Contributor did not properly post their Polkadot or Kusama address. Make sure the pull request has: "{network} address: {address}".

dvdplm · 2021-11-04T22:09:22Z

Hmm, stupid bot. We'll get this sorted.

shawntabrizi · 2021-11-05T11:08:46Z

/tip medium

substrate-tip-bot · 2021-11-05T11:08:57Z

A medium tip was successfully submitted for DefinitelyNotHilbert (13bL3pENnNbFKYD6GPjibJHv4jUTnwNPoVjp9YpU4oJfFefS on polkadot).

https://polkadot.js.org/apps/#/treasury/tips

DefinitelyNotHilbert and others added 5 commits October 26, 2021 11:48

feat(proc_macro): add support for map arguments

7ec7362

feat(proc_macro): formatting

c445ca9

feat(proc_macro): fix issues with Into trait

b59ab0e

feat(proc_macro): param_format for methods

edbebb6

Merge branch 'paritytech:master' into proc-mac-support-map-param

4b0ddde

niklasad1 reviewed Oct 30, 2021

View reviewed changes

proc-macros/src/rpc_macro.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 30, 2021

View reviewed changes

proc-macros/src/rpc_macro.rs Outdated Show resolved Hide resolved

feat(proc_macro): improve param_format checking

ee80476

- Addressed @niklasad1's suggestion to use an Option instead of just defaulting to "array".

niklasad1 reviewed Oct 31, 2021

View reviewed changes

proc-macros/src/rpc_macro.rs Outdated Show resolved Hide resolved

feat(proc_macro): apply suggestions, add test case

c374883

- Use enum for param format. - Extract parsing logic into separate function. - Add ui test.

niklasad1 approved these changes Oct 31, 2021

View reviewed changes

niklasad1 requested review from maciejhirsz and dvdplm October 31, 2021 16:08

feat(proc_macro): run cargo fmt

c593a92

dvdplm reviewed Nov 1, 2021

View reviewed changes

proc-macros/tests/ui/correct/param_format.rs Outdated Show resolved Hide resolved

proc-macros/src/render_client.rs Outdated Show resolved Hide resolved

proc-macros/src/attributes.rs Outdated Show resolved Hide resolved

proc-macros/src/render_client.rs Outdated Show resolved Hide resolved

feat(proc_macro): address suggestions

de01dd8

DefinitelyNotHilbert force-pushed the proc-mac-support-map-param branch from 47a7fca to de01dd8 Compare November 2, 2021 20:01

feat(proc_macro): document param_kind argument

f6a45cd

dvdplm approved these changes Nov 3, 2021

View reviewed changes

maciejhirsz suggested changes Nov 3, 2021

View reviewed changes

DefinitelyNotHilbert and others added 2 commits November 3, 2021 12:01

feat(proc_macro): consistent spacing

2cbe674

Apply @maciejhirsz formatting suggestion. Co-authored-by: Maciej Hirsz <1096222+maciejhirsz@users.noreply.github.com>

feat(proc_macro): apply suggestions

5c65ac8

- make parameter encoding DRY - remove strings from param_kind - return result from parse_param_kind

maciejhirsz approved these changes Nov 3, 2021

View reviewed changes

feat(proc_macro): formatting

af1eafc

maciejhirsz merged commit ff3337b into paritytech:master Nov 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proc mac support map param #544

Proc mac support map param #544

DefinitelyNotHilbert commented Oct 29, 2021 •

edited by shawntabrizi

niklasad1 left a comment •

edited

DefinitelyNotHilbert commented Oct 31, 2021

niklasad1 left a comment

niklasad1 commented Oct 31, 2021

dvdplm left a comment

maciejhirsz left a comment •

edited

maciejhirsz Nov 3, 2021

DefinitelyNotHilbert Nov 3, 2021 •

edited

maciejhirsz Nov 3, 2021 •

edited

maciejhirsz left a comment

dvdplm commented Nov 3, 2021

substrate-tip-bot bot commented Nov 3, 2021

dvdplm commented Nov 3, 2021

DefinitelyNotHilbert commented Nov 4, 2021

dvdplm commented Nov 4, 2021

substrate-tip-bot bot commented Nov 4, 2021

dvdplm commented Nov 4, 2021

shawntabrizi commented Nov 5, 2021

substrate-tip-bot bot commented Nov 5, 2021

-pub(crate) fn parse_param_kind(arg: Result<Argument, MissingArgument>) -> ParamKind {
-	match optional(arg, Argument::string).unwrap().as_deref() {
-		None | Some("array") => ParamKind::Array,
-		Some("map") => ParamKind::Map,
-		err => panic!("param_kind must be either `map` or `array`, got {:?}", err),
-	}
-}
+pub(crate) fn parse_param_kind(arg: Result<Argument, MissingArgument>) -> syn::Result<ParamKind> {
+	let kind: Option<syn::Ident> = optional(arg, Argument::value)?;
+	match kind {
+		None => Ok(ParamKind::Array),
+		Some(ident) if ident == "array" => Ok(ParamKind::Array),
+		Some(ident) if ident == "map" => Ok(ParamKind::Map),
+		Some(ident) => Err(Error::new(ident.span(), "param_kind must be either `map` or `array`")),
+	}
+}

Proc mac support map param #544

Proc mac support map param #544

Conversation

DefinitelyNotHilbert commented Oct 29, 2021 • edited by shawntabrizi

niklasad1 left a comment • edited

Choose a reason for hiding this comment

DefinitelyNotHilbert commented Oct 31, 2021

niklasad1 left a comment

Choose a reason for hiding this comment

niklasad1 commented Oct 31, 2021

dvdplm left a comment

Choose a reason for hiding this comment

maciejhirsz left a comment • edited

Choose a reason for hiding this comment

maciejhirsz Nov 3, 2021

Choose a reason for hiding this comment

DefinitelyNotHilbert Nov 3, 2021 • edited

Choose a reason for hiding this comment

maciejhirsz Nov 3, 2021 • edited

Choose a reason for hiding this comment

maciejhirsz left a comment

Choose a reason for hiding this comment

dvdplm commented Nov 3, 2021

substrate-tip-bot bot commented Nov 3, 2021

dvdplm commented Nov 3, 2021

DefinitelyNotHilbert commented Nov 4, 2021

dvdplm commented Nov 4, 2021

substrate-tip-bot bot commented Nov 4, 2021

dvdplm commented Nov 4, 2021

shawntabrizi commented Nov 5, 2021

substrate-tip-bot bot commented Nov 5, 2021

DefinitelyNotHilbert commented Oct 29, 2021 •

edited by shawntabrizi

niklasad1 left a comment •

edited

maciejhirsz left a comment •

edited

DefinitelyNotHilbert Nov 3, 2021 •

edited

maciejhirsz Nov 3, 2021 •

edited