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

Refactor Request/Resonse's TryFrom impl by generating match stmt #109

Merged
merged 7 commits into from
Jul 7, 2020

Conversation

DevinR528
Copy link
Member

No description provided.

@DevinR528
Copy link
Member Author

I wonder if this is worth doing until try is finally (:smile:) part of the language? I agree it cleans up repetitive parts of the code, I worry that it's at the expense of readability of the macro. This is by far the most complicated part of ruma that consumers sometimes use. These explicit matches are a good clue to what's going on in the generated code.

Just a thought, I'll still work on resolving the issues.

@jplatte
Copy link
Member

jplatte commented Jul 6, 2020

So you think the existing code was actually more readable? I have to say I'm not entirely convinced about the changes so far either.. :/

@DevinR528
Copy link
Member Author

Yeah, I guess after tinkering with it I would say it was more readable before. My preference (for readability) would be putting as big a code block as possible inside the quote! call, the less broken up the to-be-generated code the better.

@jplatte
Copy link
Member

jplatte commented Jul 6, 2020

Okay. Do you think the macro_rules! solution would overall improve readability, though?

@DevinR528
Copy link
Member Author

DevinR528 commented Jul 6, 2020

Removing the inner quote! would help

quote! {
    let request_body: RequestBody =
    ruma_api::try_deserialize!(
        ruma_api::exports::serde_json::from_slice(request.body().as_slice()),
        ::ruma_api::error::RequestDeserializationError,
    );
}

Or try_request_deserialize! and try_response_deserialize! macros instead of passing the error?

@jplatte
Copy link
Member

jplatte commented Jul 6, 2020

I think try_deserialize!(request, ...) and try_deserialize(response, ...) would be the shortest solution that would actually work (I think request or response has to be part of the macro input so it can be referred to properly – if you don't get it working ping me on Matrix, this could be tricky).

($kind:ident, $call:expr $(,)*) => {
::ruma_api::try_deserialize!(@ $kind, $kind, $call)
};
(@ request, $kind:ident, $call:expr) => {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, you made it work! :)

I think the way I've seen this written usually is like this: @request, so without the extra space. Where did you get the idea with the @?

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 remember reading somewhere that the @ was meant to signal internal details of the macro, if you meant with spaces I probably added that as I can't remember exactly where I saw that.

I was surprised that doubling the token $kind, $kind actually worked. Did you have a specific technique in mind?

Copy link
Member

@jplatte jplatte Jul 6, 2020

Choose a reason for hiding this comment

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

I remember reading somewhere that the @ was meant to signal internal details of the macro

Okay :)
Yeah that's what I use too and would basically be my reasoning as well. So no, I didn't have anything else in mind, just the same thing without the extra space, like you've implemented now.

I was surprised that doubling the token $kind, $kind actually worked.

Why not? ^^

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

CI failed because of formatting, please fix.

@jplatte jplatte merged commit 4ff6c6e into ruma:master Jul 7, 2020
@DevinR528 DevinR528 deleted the tryfrom-macro branch May 3, 2021 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants