Skip to content

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
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