Unify handling of file & inline snapshots

mitsuhiko / insta

A snapshot testing library for rust

https://insta.rs

Apache License 2.0

2.15k stars 98 forks source link

Unify handling of file & inline snapshots #468

Closed max-sixty closed 4 weeks ago

max-sixty commented 6 months ago

This is the code for https://github.com/mitsuhiko/insta/issues/456#issuecomment-1979089438, as mentioned in https://github.com/mitsuhiko/insta/pull/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

mitsuhiko commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

max-sixty commented 4 weeks ago

Closed in favor of #528