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

Allow minor changes in enum-map #68

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

Conversation

mr-celo
Copy link

@mr-celo mr-celo commented Oct 6, 2023

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 b7bd91f#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542R46

@rbtcollins
Copy link
Collaborator

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
Copy link
Author

mr-celo commented Oct 6, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@mr-celo
Copy link
Author

mr-celo commented Oct 11, 2023

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
Copy link
Collaborator

It is a feature request for enum-map, but as that bug says, strum made it work quite well - Peternator7/strum#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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants