-
Notifications
You must be signed in to change notification settings - Fork 26
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
add NewTypeKey derive macro #88
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM! A couple nitpicks if you want them, but this delivers a perfectly good feature IMO.
@@ -2,43 +2,22 @@ | |||
Procedural macros helper for interacting with cw-storage-plus and cosmwasm-storage. | |||
|
|||
For more information on this package, please check out the | |||
[README](https://github.com/CosmWasm/cw-plus/blob/main/packages/storage-macro/README.md). | |||
[README](https://github.com/CosmWasm/cw-storage-plus/blob/main/macros/README.md). |
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.
Nice catch!
} | ||
} | ||
}; | ||
index_list::index_list(attr, item) |
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.
Nice, thanks for moving this to a separate module
} | ||
|
||
#[test] | ||
fn newtype_works() { |
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.
My instinct with tests would be to verify a newtype behaves the same as the inner key, but it's not a hill I'd die on
use syn::{parse_macro_input, Data, DeriveInput, Fields}; | ||
|
||
pub fn cw_storage_newtype_key_derive(input: TokenStream) -> TokenStream { | ||
let input = parse_macro_input!(input as DeriveInput); |
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 wonder if you can parse it directly as ItemStruct (or whatever the type is called in syn
). Could save some of the pattern matching later.
}; | ||
|
||
let inner_type = inner_type | ||
.expect("NewTypeKey can only be derived for newtypes (tuple structs with 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.
There are ways to display macro errors similarly to other rust compiler errors, with source code being highlighted and all. I don't think it's a must, but if you're interested: https://docs.rs/syn/latest/syn/struct.Error.html
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 probably find an example later
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.
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.
thanks, I don't have a lot of experience writing macros, happy to learn better techniques. Reminding myself to address this and other comments early next week if not later today :)
No description provided.