-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add wrap
option to macro to optionally generate wrapper types for enum variants
#51
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
ee85e63
to
2f9c4ca
Compare
So this is the most recent version? Tried it out, and seems to work fine. Generation of the message enum is optional? What exactly does the macro do if you don't specify the message parameter? I tried the no_rpc flag. It seems that the only thing the no_rpc flag controls so far is generating the remote service impl. I tried the no_spans flag, but there are still spans in the enums. If we want to have no_spans at all, it should make sure that there are no spans in the enum cases. Otherwise looks good. If @matheus23 likes it maybe I can get warm with the wrap flag, even though I will probably rarely use it myself. |
9c6c086
to
9c34db8
Compare
Rebased on main now that #46 is merged |
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.
So, I gave this a quick try, and it does work as advertised.
Left a few comments regarding error messages.
This might be a nice option for people that don't like verbosity, but please let's not rewrite large protos like blobs in this style just yet.
There are various cases where having separate user-defined types for each case is way less confusing even if it is a few more LOC.
irpc-derive/src/lib.rs
Outdated
_ => { | ||
return error_tokens( | ||
variant.span(), | ||
"Each variant must have exactly one unnamed field", |
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.
Since we now support named variants, this should be something like "unnamed variants must have exactly one field.
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.
For the case where the wrap option is not set, and the enum case is a named case, we should probably mention the wrap option in the error.
…ode (#55) This has a couple of refactorings on top of #51: * Move the `wrap` functionality into a `wrap` attribute on variants, and adjust docs accordingly. I think this is better, it is a separate feature from the rpc types, and the rpc attr easily gets too long for a single line otherwise. * Add support to set visibility and additional derives for `wrap` types. This makes this far more useful, otherwise you can't use the wrap argument as soon as you need a derive (e.g. Clone) on the generated type. By refactoring the code a bit, this barely adds more code to the macro. * Make `tx` argument in `rpc` attribute optional, and change parsing to not use a map but just look for the only two arguments (`rx` and `tx`) * Refactor the derive crate to be a bit simpler, and easier to follow by putting the main macro up to the top of the file * Adds a full example to crate docs The refactor of the `derive` crate has no changes apart from the `wrap` changes outlined above.
This adds an optional
wrap
attribute to the macro. When set, it will take the enum fields (can be any kind: unit, named, tuple), turn them into a struct, and modify the enum to have a single field as usually expected by irpc.This reduces the boiler plate a lot for simple protocols. Also it allows you to skip the tedious newtyping if you e.g. have a lot of different requests that all take a single arg of the same type (e.g. a Uuid or a Hash).
It is purely optional, and works quite well with editors thanks to the span support (jump to definition jumps to the value of the wrap type, hover over that type gives the full fields, and jump to definition on a field of the generated struct takes you directly to the field on the enum variant even).
See the
simple.rs
example in the diff for the full boilerplatefree irpc-iroh galore.I.e. it turns this
into this