Skip to content
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] force proc macro api to return Result #435

Merged
merged 19 commits into from
Aug 18, 2021

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Aug 13, 2021

A little controversial solution to #403 (comment), to require each RPC method in the proc macro API to require the method to return JsonRpcResult<T, jsonrpsee_types::Error>

types/src/error.rs Outdated Show resolved Hide resolved
@niklasad1 niklasad1 marked this pull request as ready for review August 17, 2021 09:17
The rationale is to make it possible for users to either use anyhow::Error or use the helper methods.
Comment on lines 209 to 214
pub fn register_method<R, F, E>(&mut self, method_name: &'static str, callback: F) -> Result<(), Error>
where
Context: Send + Sync + 'static,
R: Serialize,
F: Fn(RpcParams, &Context) -> Result<R, CallError> + Send + Sync + 'static,
F: Fn(RpcParams, &Context) -> Result<R, E> + Send + Sync + 'static,
E: Into<Error>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have three generics is perhaps a good time to use more explicit naming. I also think the more logical order would be F, R, E perhaps?
How about:
F –> Method,
R –> RetVal,
E –> MethodError

Copy link
Member Author

@niklasad1 niklasad1 Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we put F, R, E, you will probably be annoyed

Because F the closure that you provide is always known by the compiler so in practice you would have to do this:

module.register_method::<_, (), Error>("hello_world", |_: RpcParams, _| Ok(())).unwrap();

isn't <R,E,F> better then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided the revert the Into<Error> as this doesn't really work out-of-the-box when having the client in the API.

Basically, Client::request -> Error then we would require the custom error type to implement both From<CustomError> and From<jsonrpsee::Error, I prefer to tackle it another PR to actually make it work properly.

@niklasad1 niklasad1 merged commit 09abbaa into master Aug 18, 2021
@niklasad1 niklasad1 deleted the na-require-proc-macro-api-to-return-result branch August 18, 2021 10:03
niklasad1 added a commit that referenced this pull request Sep 8, 2021
Similar to #435 that adds the same restrictions to subscriptions too.
To avoid having faulty trait bounds on when the subcription actually returns Result.
niklasad1 added a commit that referenced this pull request Sep 9, 2021
Similar to #435 that adds the same restrictions to subscriptions too.
To avoid having faulty trait bounds on when the subcription actually returns Result.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants