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

Robust crate names (alternative to #61) #62

Closed
wants to merge 11 commits into from

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Feb 2, 2021

An alternative to #61

The difference here is that the crate Idents are static items, still created using @bkchr's https://github.com/bkchr/proc-macro-crate and wrapped in a Lazy, and then wrapped in a IdentHelper that implements quote::ToTokens for ergonomic use in the derive macro.

@bkchr
Copy link
Member

bkchr commented Feb 2, 2021

But why? :D

@dvdplm
Copy link
Contributor Author

dvdplm commented Feb 3, 2021

But why? :D

It's a bit more ergonomic to use a global imo. After all, the crate name is constant and in #61 you have to pass around the crate name to all method calls which to me is a bit boiler plate-ey.

Why do you think this is worse, what are the downsides? :)

@Robbepop
Copy link
Contributor

Robbepop commented Feb 3, 2021

It's a bit more ergonomic to use a global imo. After all, the crate name is constant and in #61 you have to pass around the crate name to all method calls which to me is a bit boiler plate-ey.

I can totally understand your argument and I think there is absolutely nothing wrong with your opinion about this.
To me it just feels odd having global state in a program that does not necessarily need it - maybe that's me being religious.
Another minor downside is the extraneous once_cell dependency.

@ascjones
Copy link
Contributor

ascjones commented Feb 3, 2021

The derive code has become more complicated and could do with some refactoring, but personally I would prefer moving towards something like the 2 step parse -> Intermediate Representation -> expand style, and have the crate names defined as part of the IR.

I did consider doing some refactoring as part of #61 but decided passing to the arguments to the functions wasn't so bad.

}
}
/// Get the name of the scale-info crate, to be robust against renamed dependencies.
static SCALE_INFO: IdentHelper = IdentHelper(Lazy::new(|| {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a (probably irrational) aversion to global singletons unless absolutely necessary, though there is nothing wrong with them per se they are often a code smell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conceptually these aren't really globals of the bad kind though; they are constants and not really "state" (and obviously not mutable state).
Imagine a parallel universe where an Ident as actually a &'static str and proc-macro-crate::crate_name was a const fn, wouldn't it be more natural to have const SCALE_INFO: Ident = proc-macro-crate::crate_name("scale-info").expect("wut?!");? The crate name used is a value that does not vary, it's constant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your point, but like I said it's irrational 😁

@dvdplm
Copy link
Contributor Author

dvdplm commented Feb 3, 2021

decided passing to the arguments to the functions wasn't so bad.

It's obviously a minor point and I won't make a fuss if the consensus is this PR is not good.

My counter argument here is that function signatures carry semantic value and that going beyond three arguments whose meaning is obvious from context is best avoided when possible. In #61 we go from 3 to 5 and the newcomers are technical in nature and has little to do with what the function actually does, in other words, they're noise. So there's one code smell against the other.

I like your idea of having an explicit "expand" phase where technical details such as this one can roam free.

@dvdplm dvdplm closed this Feb 3, 2021
@ascjones
Copy link
Contributor

ascjones commented Feb 3, 2021

I agree the passing around of the args is less than ideal, this derive code is in need of some love

@ascjones
Copy link
Contributor

ascjones commented Feb 3, 2021

See #63

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.

4 participants