Unleash / unleash-client-rust

Unleash client SDK for Rust language projects
Apache License 2.0
23 stars 17 forks source link

Allow minor changes in `enum-map` #68

Open mr-celo opened 1 year ago

mr-celo commented 1 year ago

About the changes

Replace the ~ with the default ^ so to unlock minor updates in enum-map. The minor being lock may be too strict (?) and might (and in fact does) prevent users from updating their dependencies.

Discussion points

It compiles, but I am unsure about the reason for locking the minor in the first place. Introduced here https://github.com/Unleash/unleash-client-rust/commit/b7bd91f3190300f747b21a3b88789b1a8d1ce410#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542R46

rbtcollins commented 1 year ago

So the locking was a response to a bug in enum-map - long fixed I think - combined with the limitation that its macro (at that time) could not be re-exported (and we haven't tested since to see if it can be now) - so the crate version of enum-map is part of the public API of unleash-rust-client.

~ gives us major and minor control over the crate; and what we need to solve for is avoiding all the traps in https://doc.rust-lang.org/cargo/reference/resolver.html#version-incompatibility-hazards

I think rather than changing this, bumping the version to 2.6.3, and investigating the ability to re-export the derive macro (which is now in its own crate!) would be great

mr-celo commented 1 year ago

It is not possible to get enum_map_derive to work by re-exporting the proc_macro (or the crate itself)

enum_map does re-export enum_map_derive, however it seems like the proc_macro_derive implementation requires that the crate is actually imported by the crate. Code referencing ::enum_map is present throughout enum_map_derive (one example)

rbtcollins commented 1 year ago

Yah, I had opened a bug at the time, but the maintainer has moved bug trackers twice since then I think, so probably we need to file it all over again.

rbtcollins commented 1 year ago

Hmm, can't find that bug - since you've done the recent revalidation, could you perhaps file it upstream?

mr-celo commented 1 year ago

Hmm, can't find that bug - since you've done the recent revalidation, could you perhaps file it upstream?

I could, but I probably need to understand better why this is a bug. From what I understand, by using ::enum_map they are actually doing the right thing and allowing users to create aliases to the crate. Otherwise, if they used something like $crate::enum_map, they need to depend on enum_map from enum_map_derive. The report on this topic from this issue seems more like a feature request 🤔

The other point to address is the msrv. How should that be addressed? Do you wish to bump it, or should depend on a compatible version of enum_map?

rbtcollins commented 1 year ago

It is a feature request for enum-map, but as that bug says, strum made it work quite well - https://github.com/Peternator7/strum/pull/170

for instance

-  impl #impl_generics ::strum::EnumCount for #name #ty_generics #where_clause {
+ impl #impl_generics #strum_module_path::EnumCount for #name #ty_generics #where_clause {

note the addition of a concrete module path; which can be used when using the macro. So for instance

#[strum(crate_path = "some_crate::strum")]

Or for us, we'd document

#[derive(EnumMap)]
#[enum_map(crate_path = "unleash_api_client::enum_map")

.. or something like that.

rbtcollins commented 1 year ago

For the MSRV, that looks like they have a hard dep. I think raising to 1.61 is fine, thats 10 releases ago now.