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

Upgrade syn to 2.0 #404

Closed
wants to merge 1 commit into from
Closed

Upgrade syn to 2.0 #404

wants to merge 1 commit into from

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Jul 11, 2023

Didn't convert the huge macro_rules! macro and some non-trivial NestedMeta usage. I'd rather just convert it to parsing in one step like I did for match_attribute_with_str_list_value, but (a) I expected this to be a short contribution and have run out of time for now and (b) I'm not sure this is even what you want, so let's discuss how to proceed from this draft PR.

Resolved #401.

@zeenix
Copy link
Contributor

zeenix commented Jul 27, 2023

@bilelmoussaoui any chance you can take over?

@zeenix
Copy link
Contributor

zeenix commented Aug 17, 2023

I'm not sure this is even what you want,

Can you please elaborate on this?

@jplatte
Copy link
Contributor Author

jplatte commented Aug 17, 2023

I'm talking about this change. It impacts error messages, e.g. supplying attribute_argument = true for an attribute_argument that expects a string value would previously result in a compilation error saying "invalid literal type for attribute_argument attribute", and now will likely say something like "syntax error, expected "" (pointing to true). Are you okay with error reporting changing to syn's parse errors, from the custom errors based on a parse result?

@zeenix
Copy link
Contributor

zeenix commented Aug 17, 2023

Thanks for explaining. 👍

Are you okay with error reporting changing to syn's parse errors, from the custom errors based on a parse result?

Yeah. The new error seems more descriptive even. My preference would always bee what's most helpful to the user.

@jplatte jplatte force-pushed the syn2 branch 2 times, most recently from b20513e to 562b76d Compare August 23, 2023 11:25
@jplatte jplatte changed the title WIP: Upgrade syn to 2.0 Upgrade syn to 2.0 Aug 23, 2023
@jplatte jplatte marked this pull request as ready for review August 23, 2023 11:25
Comment on lines +114 to +117
// Since signals do not have a function body, they don't parse as ImplItemFn…
ImplItem::Verbatim(tokens) => {
// … thus parse them ourselves and construct an ImplItemFn from that
let decl = syn::parse2::<ImplItemSignal>(tokens.clone())?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the hardest piece to find out. Apparently syn 1.x just accepted ImplItemMethods without a body, but syn 2.x does not. The error message when re-emitting this ImplItem::Verbatim was plenty confusing (it was "expected impl" on the first token of the item).

@zeenix
Copy link
Contributor

zeenix commented May 18, 2024

Closing in favour of #798.

@zeenix zeenix closed this May 18, 2024
zecakeh added a commit to zecakeh/zbus that referenced this pull request May 19, 2024
Built on top of and replaces dbus2#404.
Fixes dbus2#401.

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
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.

Switch to syn2
2 participants