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

Add support for syn 2.0.40's new Expr::Group to dsl::auto_type #3873

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

Ten0
Copy link
Member

@Ten0 Ten0 commented Dec 11, 2023

This expression may get yielded when parsing $a: expr from other macros.

@Ten0 Ten0 requested a review from a team December 11, 2023 10:46
@Ten0 Ten0 self-assigned this Dec 11, 2023
@Ten0 Ten0 force-pushed the auto_type_syn_2_40 branch from acc3811 to 89c1feb Compare December 11, 2023 10:47
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

The change itself looks good 👍

I would prefer to have a some sort of test somewhere that verifies that the code compiles.

Also I would like to use the possibility to point again to #3783 for the PR that fixes most of the type aliases to work with auto_type.

@Ten0
Copy link
Member Author

Ten0 commented Jan 12, 2024

There aren't tests for #[dsl::auto_type] besides the doctests so far and that doesn't feel worth putting in the doc, and unfortunately this slipped out of my mind and I can't remember the precise specifics of how to reproduce.
But that did fix my issue in a macro and matches the syn documentation so I'm confident enough that this is correct and we're unlikely to break it in the future.
I may add a test for this in the test file from the other PR but for now I think it would be unreasonable to dedicate that amount of investigation/merge time for such a small thing.

@Ten0 Ten0 added this pull request to the merge queue Jan 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 12, 2024
@weiznich
Copy link
Member

weiznich commented Jan 12, 2024

That sounds ok. I always forget that the #3783 is not merged yet as that PR adds a bunch of "tests".

EDIT: I've rebased this branch to make the failing CI less likely.

This expression may get yielded when parsing `$a: expr` from other macros.
@weiznich weiznich enabled auto-merge January 12, 2024 12:27
@weiznich weiznich added this pull request to the merge queue Jan 12, 2024
Merged via the queue into diesel-rs:master with commit 5f173ab Jan 12, 2024
37 checks passed
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.

2 participants