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/Response TryFrom impl to allow '?' on fallible calls #108
Conversation
@@ -77,7 +77,7 @@ pub fn expand_event(input: DeriveInput) -> syn::Result<TokenStream> { | |||
|
|||
let event_type = ::ruma_events::EventContent::event_type(&self.content); | |||
|
|||
let mut state = serializer.serialize_struct(stringify!(#ident), 7)?; | |||
let mut state = serializer.serialize_struct(stringify!(#ident), 9)?; |
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.
I can remove this I just wanted to remember to ask if we should be dealing with this and is there any chance this could be part of the bincode deserialization problem @MTRNord was asking about on matrix (either in the Ruma room or in matrix-rust-sdk)?
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.
@jplatte had some ideas why bincode doesn't work I think. json does work fine (obviously. this is matrix. if it doesn't do json this wouldn't be matrix).
I still dont understand why it doesn't work with bincode :D I just removed bincode from my project
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.
No, this has nothing to do with the bincode "expected valid JSON" error. If you want more details about that, ping me on Matrix.
@@ -65,7 +65,7 @@ impl Response { | |||
} | |||
ResponseField::NewtypeRawBody(_) => { | |||
quote_spanned! {span=> | |||
#field_name: response.into_body() | |||
#field_name: resp.body().to_vec() |
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.
I was wondering whether there would be a case like this. This now being a full copy of the response makes my whole idea moot unfortunately¹. I don't want to have worse runtime performance for somewhat nicer generated code.
I have another idea though: We could hide the repetitive
match <expr> {
Ok(val) => val,
Err(e) => return <error type>::new(<actual error>, <reponse / request>).into(),
}
behind a helper function (or two, one for requests and one for responses). I hope that would make the macro code easier to read.
Does that makes sense to you? Do you have other ideas for how to make the code more readable?
¹: If you want a more in-depth explanation of why I don't think this can work (without try
blocks
Work toward issue #6