-
Notifications
You must be signed in to change notification settings - Fork 55
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
Increase syn dependency lower bound to 2.0.0 #3682
Conversation
stratisd_proc_macros compiles...but panics. |
Now panicking in a better place... |
compiles |
cba8b49
to
d9a5d3e
Compare
If it passes, it should be good. |
stratisd_proc_macros/src/lib.rs
Outdated
.filter(|(_, a)| a.path().is_ident("pool_mutating_action")) | ||
.map(|(i, a)| { | ||
if let Meta::List(MetaList { ref tokens, .. }) = a.meta { | ||
if let TokenTree::Literal(litstr) = tokens.clone().into_iter().next().expect("stratisd code always supplies string literal argument to pool_mutating_actions") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be further destructured using this enum. It will allow you to avoid converting to a String and then trimming the quotation marks, instead just getting the String value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
litstr type is TokenTree::Literal, which is a struct with private fields and a rather awkward set of methods in syn2. I think that syn2 probably expects that the normal way to encode that argument value is without the '"' in the first place, as a plain Ident, but I don't want to hit every spot where the pool_mutating_action
annotation is used to change that, so I left it alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. I think changing this to use an ident would be better after looking at the types. Would you be willing to do that? I think it would generally be better from the standpoint of upgrading to the new API. I'll also look into the recommended way of doing this because I have some other crates I maintain that need updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would, but I'ld like to postpone that until we start the next release. I'll file an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay sounds good!
@jbaublitz Ok, it is ready again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm retracting my approval just to suggest one more possible avenue. Does parse_nested_meta do what we want here? Maybe not. I'll look into it more when I get back, but it looks promising.
528adba
to
f57a07c
Compare
@jbaublitz The last commit tightens it up a bit. It can be tightened up even further by specifying parse_args with Ident type, but that requires changing all the uses every where to actually be Idents, not strings. |
Would you mind holding this until I get back? I want to investigate further. |
@jbaublitz Yeah, that's fine. |
@mulkieran I found a better way to do it:
This requires no changes to the way we currently provide the rejection level. |
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
Yeah. That's a better way to parse. But the expect is still rejected, so I'll stick with panic. |
What's rejecting it? |
It's a correct clippy lint: https://rust-lang.github.io/rust-clippy/master/index.html#/expect_fun_call . With an expect, the error message is eagerly evaluated. With the panic, it is not. |
Panic always if no argument for pool_mutating_actions. Panic if more than one of the designated attribute found. Signed-off-by: mulhern <[email protected]>
squashed. |
Closes #3650