Allow user to set the crate path of num_enum #77

yshui · 2022-09-12T22:56:39Z

Add a #[num_enum(crate = ..)] attribute, which lets the user set where the num_enum crate is located.

yshui · 2022-09-13T09:59:47Z

This closes #76

danielhenrymantilla

Great, thanks for the quick PR! 🙏

I have some nits here and there, but otherwise LGTM ✅

danielhenrymantilla · 2022-09-13T10:46:41Z

num_enum/tests/try_from_primitive.rs

+mod reexport {
+    pub use ::num_enum;
+}


Could you have this on its own tests/file.rs, and then, use, instead:

Suggested change

mod reexport {

pub use ::num_enum;

}

extern crate std as num_enum; // ensure `::num_enum` is a bad path

mod reexport {

pub extern crate num_enum;

}

This way if the macro decided to ignore the crate parameter, we will detect it and fail 🙂

danielhenrymantilla · 2022-09-13T10:49:41Z

num_enum_derive/src/lib.rs

+        let s: syn::LitStr = input.parse()?;
+        let path = s.parse_with(syn::Path::parse_mod_style)?;


Suggested change

let s: syn::LitStr = input.parse()?;

let path = s.parse_with(syn::Path::parse_mod_style)?;

let path = syn::Path::parse_mod_style(input)?;

The fact serde uses a literal on the rhs of crate = … is a historical artifact (crate = ::some_path was not legal at the beginning), but nowadays not requiring code to be stringified is better for spans and IDEs 🙂

danielhenrymantilla · 2022-09-13T11:01:18Z

num_enum_derive/src/lib.rs

+            for attr in attrs {
+                if attr.path.is_ident("num_enum") {
+                    match attr.parse_args_with(NumEnumAttributes::parse) {
+                        Ok(NumEnumAttributes { items }) => {
+                            if items.len() != 1 || crate_path.is_some() {
+                                die!(attr.tokens => "Expected exactly one `crate` argument");
+                            } else {
+                                let NumEnumAttributeItem::Crate(item) =
+                                    items.into_iter().next().unwrap();
+                                crate_path = Some(item.path);
+                            }
+                        }
+                        Err(err) => {
+                            die!(attr =>
+                                format!("Invalid attribute: {}", err)
+                            );
+                        }
+                    }


This won't "scale well" if other attributes are added. I'd rather suggest an approach along the lines of .flat_map()ing / nested-for-style the attributes (since #[num_enum(foo)] #[num_enum(bar)] is the same as #[num_enum(foo, bar)]:

for attr in attrs { if attr.path.is_ident("num_enum") { for item in attr.parse_args_with(NumEnumAttributes::parse)? { match item { // when written like this, it's easier to extend with more attrs. NumEnumAttributeItem::Crate(item) => { if crate_path.is_some() { die!(item.path => "duplicate `crate` annotation"); } crate_path = Some(item.path); }, } } } … }

danielhenrymantilla · 2022-09-13T11:02:56Z

num_enum_derive/src/lib.rs

+        syn::LitStr::new(&format!("::{}", get_crate_name()), Span::call_site())
+            .parse_with(syn::Path::parse_mod_style)
+            .unwrap()


Suggested change

syn::LitStr::new(&format!("::{}", get_crate_name()), Span::call_site())

.parse_with(syn::Path::parse_mod_style)

.unwrap()

let krate = get_crate_name();

parse_quote!( :: #krate )

this parse_quote! would generate :: "num_enum" because krate is a String

maybe let krate = format_ident!("{}", get_crate_name());?

Ah true, yeah what you've just suggested would be the right thing.

Although at that point, I guess we could go back to something closer to your original suggestion:

syn::parse_str(&format!("::{}", get_crate_name())).unwrap()

(it just sidesteps the intermediary LitStr)

danielhenrymantilla · 2022-09-13T11:04:03Z

num_enum_derive/src/lib.rs

+    let krate = enum_info.crate_path.clone().unwrap_or_else(|| {
+        syn::LitStr::new(&format!("::{}", get_crate_name()), Span::call_site())
+            .parse_with(syn::Path::parse_mod_style)
+            .unwrap()
+    });


Hmm, given the code duplication, I wonder if get_crate_name() shouldn't be taking a Option<Path> parameter and being the one emitting the actual path?

Add a `#[num_enum(crate = ..)]` attribute, which lets the user set where the num_enum crate is located.

yshui · 2023-01-11T16:35:58Z

@danielhenrymantilla hey, sorry for taking a while to get back to this. i think i have now addressed most of the points.

danielhenrymantilla

Great!

illicitonion

LGTM, thanks! I have suggestions for a few very small tweaks, then let's merge!

illicitonion · 2023-01-15T00:15:04Z

num_enum/tests/try_build/compile_fail/multiple_crate_path.stderr

@@ -0,0 +1,5 @@
+error: Expected exactly one `crate` argument


I'd expect this error message to be "at most one" rather than "exactly one" - it's allowed to not specify a crate argument.

illicitonion · 2023-01-15T00:15:19Z

num_enum/tests/try_build/compile_fail/variants_with_fields.stderr

@@ -1,16 +1,16 @@
-error: `num_enum` only supports unit variants (with no associated data), but `Number::NonZero` was not a unit variant.
+error: `:: num_enum` only supports unit variants (with no associated data), but `Number::NonZero` was not a unit variant.


I wouldn't expect a space between :: and num_enum here?

illicitonion · 2023-01-15T00:47:53Z

num_enum_derive/src/lib.rs

+                        match item {
+                            NumEnumAttributeItem::Crate(item) => {
+                                if crate_path.is_some() {
+                                    die!(attr.tokens => "Expected exactly one `crate` argument");


Suggested change

die!(attr.tokens => "Expected exactly one `crate` argument");

die!(attr.tokens => "Expected at most one `crate` argument");

illicitonion · 2023-01-15T00:51:22Z

num_enum_derive/src/lib.rs

+            proc_macro_crate::FoundCrate::Itself => parse_quote!(::num_enum),
+            proc_macro_crate::FoundCrate::Name(name) => {
+                let krate = format_ident!("{}", name);
+                parse_quote!( :: #krate )


Suggested change

parse_quote!( :: #krate )

parse_quote!( ::#krate )

illicitonion · 2023-01-15T00:55:23Z

num_enum_derive/src/lib.rs

 // Don't depend on proc-macro-crate in no_std environments because it causes an awkward dependency
 // on serde with std.
 //
 // no_std dependees on num_enum cannot rename the num_enum crate when they depend on it. Sorry.
 //
 // See https://github.com/illicitonion/num_enum/issues/18
 #[cfg(not(feature = "proc-macro-crate"))]
-fn get_crate_name() -> String {
-    String::from("num_enum")
+fn get_crate_name(path: Option<syn::Path>) -> syn::Path {


Suggested change

fn get_crate_name(path: Option<syn::Path>) -> syn::Path {

fn get_crate_path(path: Option<syn::Path>) -> syn::Path {

Let's rename this (and the other #[cfg-gated one, and its call sites) to make more clear that it has the :: prefix (and that callers don't need to add it)

diondokter · 2023-12-13T15:13:54Z

Can I help in any way to get this over the finish line and get this feature released?

danielhenrymantilla reviewed Sep 13, 2022

View reviewed changes

Allow user to set the crate path of num_enum

51ef39c

Add a `#[num_enum(crate = ..)]` attribute, which lets the user set where the num_enum crate is located.

yshui force-pushed the main branch from 35eea84 to 51ef39c Compare January 11, 2023 16:35

danielhenrymantilla approved these changes Jan 12, 2023

View reviewed changes

illicitonion reviewed Jan 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow user to set the crate path of num_enum #77

Allow user to set the crate path of num_enum #77

yshui commented Sep 12, 2022

yshui commented Sep 13, 2022

danielhenrymantilla left a comment

danielhenrymantilla Sep 13, 2022

danielhenrymantilla Sep 13, 2022

danielhenrymantilla Sep 13, 2022 •

edited

Loading

danielhenrymantilla Sep 13, 2022

yshui Jan 11, 2023

yshui Jan 11, 2023

danielhenrymantilla Jan 12, 2023

danielhenrymantilla Sep 13, 2022 •

edited

Loading

yshui commented Jan 11, 2023

danielhenrymantilla left a comment

illicitonion left a comment

illicitonion Jan 15, 2023

illicitonion Jan 15, 2023

illicitonion Jan 15, 2023

illicitonion Jan 15, 2023

illicitonion Jan 15, 2023

diondokter commented Dec 13, 2023

		let s: syn::LitStr = input.parse()?;
		let path = s.parse_with(syn::Path::parse_mod_style)?;

	let s: syn::LitStr = input.parse()?;
	let path = s.parse_with(syn::Path::parse_mod_style)?;
	let path = syn::Path::parse_mod_style(input)?;

		@@ -1,16 +1,16 @@
		error: `num_enum` only supports unit variants (with no associated data), but `Number::NonZero` was not a unit variant.
		error: `:: num_enum` only supports unit variants (with no associated data), but `Number::NonZero` was not a unit variant.

	die!(attr.tokens => "Expected exactly one `crate` argument");
	die!(attr.tokens => "Expected at most one `crate` argument");

	fn get_crate_name(path: Option<syn::Path>) -> syn::Path {
	fn get_crate_path(path: Option<syn::Path>) -> syn::Path {

Allow user to set the crate path of num_enum #77

Are you sure you want to change the base?

Allow user to set the crate path of num_enum #77

Conversation

yshui commented Sep 12, 2022

yshui commented Sep 13, 2022

danielhenrymantilla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhenrymantilla Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhenrymantilla Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

yshui commented Jan 11, 2023

danielhenrymantilla left a comment

Choose a reason for hiding this comment

illicitonion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diondokter commented Dec 13, 2023

danielhenrymantilla Sep 13, 2022 •

edited

Loading

danielhenrymantilla Sep 13, 2022 •

edited

Loading