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

add NewTypeKey derive macro #88

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

add NewTypeKey derive macro #88

wants to merge 2 commits into from

Conversation

dakom
Copy link

@dakom dakom commented Sep 25, 2024

No description provided.

Copy link
Contributor

@uint uint left a 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).
Copy link
Contributor

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)
Copy link
Contributor

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() {
Copy link
Contributor

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);
Copy link
Contributor

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)");
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

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 :)

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.

2 participants