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

Allow trailing commas in inline snapshots #472

Merged

Conversation

narpfel
Copy link
Contributor

@narpfel narpfel commented Apr 3, 2024

After using mitsuhiko/insta-cmd#12 for a bit, I noticed that cargo insta test would not pick up on mismatches in inline snapshots with trailing commas.

This PR fixes that and also allows trailing commas for inline snapshots in other macros.

This PR uses code from #454 by @max-sixty.

Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

Awesome @narpfel

Is the comma be removed when a snapshot is written? If so, do we want that or do we prefer to retain it?

We don't have great tests for this sort of thing IIRC, so it's worthwhile checking by hand that the replacement behavior is what's expected (or ofc +1 to adding a test...)

(@mitsuhiko is the maintainer so this is just a comment from another contributor)

@@ -321,7 +319,7 @@ macro_rules! assert_debug_snapshot {
// the value. This allows us to implement other macros with a small wrapper. All
// snapshot macros eventually call this macro.
//
// The macro handles optional trailing commas in file snapshots.
// This macro is expected to handle trailing commas.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// This macro is expected to handle trailing commas.
// This macro handles trailing commas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this? db010fb

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes cool! (FYI GH also allows "Accept suggestion")

@narpfel
Copy link
Contributor Author

narpfel commented Apr 3, 2024

Is the comma be removed when a snapshot is written? If so, do we want that or do we prefer to retain it?

The comma is kept, for example in this test: 6b28cb3#diff-42101e5a83cd4bee69fc68a5258dfbeafa0c657945e327f0aee0fd05ab309d58R35-R38

@max-sixty
Copy link
Collaborator

The comma is kept, for example in this test: 6b28cb3#diff-42101e5a83cd4bee69fc68a5258dfbeafa0c657945e327f0aee0fd05ab309d58R35-R38

Perfect!

@mitsuhiko mitsuhiko merged commit a4f96da into mitsuhiko:master May 15, 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.

3 participants