Skip to content

Conversation

@max-sixty
Copy link
Collaborator

This is the code for #456 (comment), as mentioned in #466.

There's a very small breaking change in YAML inline snapshots — shown here in the tests. In return, it cuts some complicated code from macros.rs, making them more maintainable and less likely to hit some of our recent issues, such as inconsistent support for trailing commas etc

This is the code for mitsuhiko#456 (comment), as mentioned in mitsuhiko#466.

There's a very small change in yaml inline snapshots — shown here in the tests. In return, it makes the macros simpler & more maintainable.
@mitsuhiko
Copy link
Owner

I'm not a huge fan of this. The current logic means that leading whitespace is handled better for yaml snapshots. What's the benefit of changing this?

@max-sixty
Copy link
Collaborator Author

The current logic means that leading whitespace is handled better for yaml snapshots. What's the benefit of changing this?

Couple of reasons:

  • Currently snapshots are the same for file & inline everywhere except yaml snapshots
  • I agree better handling of leading & trailing whitespace would be nice — but these are yaml snapshots — they can't have leading whitespace! (or am I wrong?). And why handle it differently between file & inline?
  • Because we're handling file & inline differently in this one place, we need to thread that arg all the way through the code, which makes the code more complicated

(not my most important contribution, won't be offended if you close! Though I also don't quite see the reason to have the existing code...)

@mitsuhiko
Copy link
Owner

Yeah this might be right. I'm generally quite hesitant to changing formatting the format. Today this choice means that every line starts with --- which means even that when you assert_yaml_snapshot!(true, ...); you end up in multi-line mode. So it's not as much that the leading whitespace is relevant as that you end up with stuff in the next line. Maybe that's not ideal but I also did not think that much to begin with.

@max-sixty
Copy link
Collaborator Author

OK! I don't think there are any cases that would be worse like this, but if you have any lingering doubts then happy to test them...

@mitsuhiko
Copy link
Owner

I think I would be okay making this change with insta 2.

@max-sixty max-sixty force-pushed the unify-file-inline branch from 039e148 to 038b8b2 Compare July 15, 2024 04:52
mitsuhiko pushed a commit that referenced this pull request Aug 31, 2024
This is a version of #468 that doesn't break compatibility — the change
in snapshot (removing the leading `---` in inline snapshots) is
optional, and snapshots still pass if they have it. Otherwise it has the
same benefits of #468

(There is a tiny corner case that if a snapshot is intended to test
whether there's a leading `---`, we could get a false match, but this is
a very small possibility — much much smaller than removing the line
endings we've done)

As part of that, this adds `as_str_exact` & `matches_fully` methods to
`SnapshotContents`, which allows us to unify our normalization of
snapshots contents, and allow detecting snapshots that pass but don't
completely match; for example if we want to restore checking for line
endings.
@max-sixty
Copy link
Collaborator Author

Closed in favor of #528

@max-sixty max-sixty closed this Aug 31, 2024
@max-sixty max-sixty deleted the unify-file-inline branch August 31, 2024 21:11
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