-
Notifications
You must be signed in to change notification settings - Fork 67
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
fix: codegen sanity for enum serialization #102
Conversation
@@ -77,7 +77,7 @@ pub fn enum_de(input: &ItemEnum, cratename: Ident) -> syn::Result<TokenStream2> | |||
if let Some(method_ident) = init_method { | |||
Ok(quote! { | |||
impl #impl_generics #cratename::de::BorshDeserialize for #name #ty_generics #where_clause { | |||
fn deserialize(buf: &mut &[u8]) -> core::result::Result<Self, #cratename::maybestd::io::Error> { | |||
fn deserialize(buf: &mut &[u8]) -> ::core::result::Result<Self, #cratename::maybestd::io::Error> { |
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.
Ideally, we'd use a name from borsh
crate here, like serde
does, but ::core
is already an improvement:
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.
And that would keep sanity for if you imported a crate named core
? That one feels more opinionated since it increases the exposed __private
API so maybe best to do it in a separate PR?
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.
That's definitely should be done in a separate PR, if at all.
Practically, I wouldn't expect this to make any difference, but it's just good hygiene (pun intended) to only rely on names provided by the runtime-support crate and not rely on any properties of the call-site environment.
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 have just noticed this PR fall through the cracks. Do you still want to merge it? If so, go ahead
structs were fixed in 985a20b but this was not done for enums.
cc @BenKurrek