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

Remove storage-plus dependency from storage-macro #817

Merged
merged 6 commits into from
Sep 29, 2022

Conversation

chipshort
Copy link
Contributor

The tests that were depending on storage-plus are now in storage-plus. I also changed the CI to actually run them.

I added the docs from readme to the actual macro. I'm not sure how docs.rs works in this regard though, since the reexport is only available with the macro feature.

closes #808

@uint
Copy link
Contributor

uint commented Sep 29, 2022

I added the docs from readme to the actual macro. I'm not sure how docs.rs works in this regard though, since the reexport is only available with the macro feature.

Good catch. You should be able to test the docs locally by running cargo doc --open --features ...

You could also add some metadata to Cargo.toml to make the docs on docs.rs include those features:
https://docs.rs/about/metadata

@chipshort
Copy link
Contributor Author

Yes, locally it was working with --all-features or --features macro, so should I just add this?

[package.metadata.docs.rs]
all-features = true

@uint
Copy link
Contributor

uint commented Sep 29, 2022

Yep, that's fine

Comment on lines 8 to 26
/// Auto generate an `IndexList` impl for your indexes struct.
///
/// # Example
///
/// ```rust
/// #[derive(Serialize, Deserialize, Clone, Debug, PartialEq)]
/// struct TestStruct {
/// id: u64,
/// id2: u32,
/// addr: Addr,
/// }
///
/// #[index_list(TestStruct)] // <- Add this line right here.
/// struct TestIndexes<'a> {
/// id: MultiIndex<'a, u32, TestStruct, u64>,
/// addr: UniqueIndex<'a, Addr, TestStruct>,
/// }
/// ```
///
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so normally macro docs would live in the storage-plus crate, above the reexport. Here's an example.

Does this example pass when tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, it does not. I'll move it to the reexport

@@ -16,6 +16,5 @@ proc-macro = true
syn = { version = "1.0.96", features = ["full"] }

[dev-dependencies]
cw-storage-plus = { version = "<=0.15.1, >=0.14.0", path = "../storage-plus" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI: the way the version requirements are specified here is a hack to make a cyclic dependency publishable to crates.io. Good thing we don't actually need it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Good that this is not needed, yes. Thanks for working on simplifying this.

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. Thanks for making sure those tests are actually run in CI

@uint uint merged commit f5e9d13 into main Sep 29, 2022
@uint uint deleted the 808-remove-circular-dependency branch September 29, 2022 12:31
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.

Remove circular dependency between storage-macro and storage-plus
3 participants