Insta 2.0 ideas?

[max-sixty]

Would be great to keep as much consistency as possible. But is there anything we want to change while we can?

A few ideas:

• Update snapshot formats — already discussed
• Do we want to attempt to make the macros simpler?
  • Currently we have (value) & (name, value) & (name, value, debug_expr) (+ redaction filters, inline, etc), which is arguably OK for experienced users, but less immediately obvious for new folks reading code.
  • We could move the debug_expr into with_settings.
  • Though the more difficult-to-understand part is that adding a value after the first value means adding it before the first value. And not sure whether there's a good solution.
• Compel match .. in redactions? This allows rustfmt to work. Plausibly it also delineates the redaction expression for easier reading.
• Would be great for inline snapshots to work with rustfmt, but don't think there's a way — if the inline snapshot is a normal expression without special syntax (such as @), it's not possible to discriminate between (name, value) and (value, snapshot)
• Align syntax of filters & redactions? They are quite similar in purpose but currently defined very differently
• Shorten snapshot file names? #377, possibly breaks too much for a very marginal benefit

[max-sixty]

This is purely internal, but along with updating the snapshot formats, if we could unify how inline & file snapshots are redacted, that makes some code simpler.

insta/src/macros.rs

Lines 228 to 229 in 2959d79

	// Note that if we could unify the Inline & File represenations of snapshots
	// redactions we could unify some of these branches.

The difference would be we don't have --- in inline snapshots, which at first glance seems unnecessary?

diff --git a/tests/test_settings.rs b/tests/test_settings.rs
index db67e1d..da0c6ce 100644
--- a/tests/test_settings.rs
+++ b/tests/test_settings.rs
@@ -17,7 +17,6 @@ fn test_simple() {
     settings.set_sort_maps(true);
     settings.bind(|| {
         assert_yaml_snapshot!(&map, @r###"
-        ---
         a: first value
         b: second value
         c: third value
@@ -40,7 +39,6 @@ fn test_bound_to_scope() {
         settings.set_sort_maps(true);
         let _guard = settings.bind_to_scope();
         assert_yaml_snapshot!(&map, @r###"
-        ---
         a: first value
         b: second value
         c: third value
@@ -62,7 +60,6 @@ fn test_settings_macro() {
 
     with_settings!({sort_maps => true}, {
         insta::assert_yaml_snapshot!(&map, @r###"
-        ---
         a: first value
         b: second value
         c: third value

[mitsuhiko]

I have been thinking about this a bit more and if we do a major move to a new version it might be a good idea to address some of the larger challenges of the library. For me the biggest is that the patching of inline snapshots is unreasonably complex and thus delegated to the cargo-insta library and actually working with snapshots is tricky. It would be nice if the basics even for inline snapshots (#490) requires a heavy dependency.

There is also sometimes the desire to work with snapshot files directly. For example #475 wants to place a snapshot external to the metadata, to track schema changes.

One way to go about it would be to move the actual management of the snapshot into a separate macro than the assertion.

Inline snapshots:

// old
assert_snapshot!("the value", @"the value");

// new
assert_snapshot!("the value", snap!("the value"));

Named snapshots:

// old
assert_snapshot!("name", "the value");
// new
assert_snapshot!("the value", snap!(named="name"));

Implicitly named:

assert_snapshot!("the value", snap!(named));

The benefit of this is that the macro (snap!) could be independently expended of the assertion macros, it captures the location accurately and no longer needs the overhead of the complex rust parser to fix the locations.

This would also allow additional functionality to be placed on the snapshot for placing it somewhere. Hypothetically for binaries:

assert_snapshot!(binary_stuff, snap!(binary, ext=".png", named="my-picture"));

Or to interact with the snapshot directly:

let snap = snap!("the-value");
assert_eq!(snap.text(), "the-value");

Another unrelated thing I wish could be cleaned up are the settings. I think they are just bad today. with_settings seems awful and I rather make this pattern nicer:

fn assert_foo() {
    // s is a scope guard that drops.  Until then the settings are reconfigured for this
    // function
    let mut s = insta::reconfigure();
    s.add_filter(r"\d{4}-\d{2}-\d{2}", "[DATE"]);
    assert_snapshot!("Today is 2024-05-15", snap!("Today is [DATE]"));
}

[max-sixty]

I will think more but some initial thoughts:

---

Can we replace inline snapshot values without cargo-insta already? Assuming the tests execute serially, can we replace the code in the source file as the tests execute, keeping track of any additional lines we add?

---

I agree the settings seem too complicated. They're split between the with_settings and the assert macro itself which contains information such as debug_expr & filters & redactions. (Re "Do we want to attempt to make the macros simpler?" above). Encapsulating the snapshot in another macro could indeed simplify that. And the ability to make it into an object let snap = snap!("the-value"); is really nice.

But snap! adds another macro, which makes insta a bit more complicated. There is a very nice simplicity to the standard inline snapshot assert_snapshot!(foo, @"bar"); — it requires zero explanation for someone to understand, without knowing anything about insta, and adding another macro possibly reduces that simplicity.

One option would be to move everything to the new snap! macro, and give it methods:

snap!("the-value").assert_debug_matches("the_value");

let s = snap!(name=foo);
s.with_redactions(...).with_snapshot_suffix(...).assert_yaml_matches(...);

---

FWIW I would find this confusing, where named isn't previously defined:

assert_snapshot!("the value", snap!(named));

An alternative could be snap!(name=None)) or snap!()) or snap!(name=Default::default());

---

One final comment — occasionally these big rewrites can be so big that they never happen, and it prevents a smaller release that would get done. You're obv the main author and the main bottleneck, so depends on your capacity. If you think we can't get something so ambitious done, possibly we could do a less ambitious change. But great if you think we can, am happy to contribute.

[mitsuhiko]

Can we replace inline snapshot values without cargo-insta already? Assuming the tests execute serially, can we replace the code in the source file as the tests execute, keeping track of any additional lines we add?

Yes, but not without pulling in the rather complex syn based parsing logic which is a huge dependency. The benefit of a snap! macro would be that it could use line! and column! to capture the precise location of the macro invocation. In that case only string tokenization needs to be implemented to replace the value.

One option would be to move everything to the new snap! macro, and give it methods:

I was thinking about this, but it turns the assertions into "reference value first", assertion later which in case of long macros makes them hard to understand. Potentially this would make more sense?

debug!(value).matches(snap!(...));

But that's even more unwieldy. I need to think about this more, but I'm at least at the moment heavily leaning towards playing around if snap! is a better option because it separates the handling of snapshots from the assertions.

occasionally these big rewrites can be so big that they never happen, and it prevents a smaller release that would get done

I'm not too worried about that.

[max-sixty]

Yes, but not without pulling in the rather complex syn based parsing logic which is a huge dependency. The benefit of a snap! macro would be that it could use line! and column! to capture the precise location of the macro invocation. In that case only string tokenization needs to be implemented to replace the value.

OK — I know you value limiting dependencies a lot. Would putting it behind a feature help allay the concern? Or it's too crucial a feature to relegate to optional?