Incorrect inline snapshot indentation with `hard_tabs = true`

mitsuhiko / insta

A snapshot testing library for rust

https://insta.rs

Apache License 2.0

2.25k stars 101 forks source link

Incorrect inline snapshot indentation with `hard_tabs = true` #679

Open nitsky opened 3 weeks ago

nitsky commented 3 weeks ago

What happened?

I am using rustfmt configured with hard_tabs = true. When insta writes inline snapshots, it writes one space character for each tab.

Reproduction steps

#[test]
fn test() {
    insta::assert_debug_snapshot!(vec![1, 2, 3], @r"
 [
     1,
     2,
     3,
 ]
 ");
}

Insta Version

1.41

rustc Version

1.82.0

What did you expect?

I expected the indentation to use the tab character, not the space character.

max-sixty commented 3 weeks ago

Thanks for the issue.

Are you referring to the content of the string? Isn't that the debug output of the vec, without any connection to formatting rust code? Very possibly I'm misunderstanding...

nitsky commented 3 weeks ago

Sorry for not being clear. When insta writes inline snapshots, it determines the indentation of the assert_snapshot invocation, then applies that as leading indentation to the snapshot, so that the snapshot lines up nicely with the assert_snapshot call. I observe that this works very well when using spaces as indentation, but not when using tabs. In the example in my comment above, I would expect the [ to line up with the i in insta. Instead, I observe that while the line with insta::assert_snapshot is indented with one tab, the snapshot content is indented with one space. It appears that insta is calculating the number of whitespace characters, then unconditionally using a space as the indentation character.

max-sixty commented 3 weeks ago

Ah sorry for indeed misunderstanding.

The root of this is here: https://github.com/mitsuhiko/insta/blob/ce0027a263317b1d6d00f568e12d871dc450c39e/insta/src/snapshot.rs#L632-L649

We're counting whitespaces. We could instead do one of:

Multiply tab characters by 4 (or 8?); it would still use spaces but would have more chance of lining up. Very small easy change.
Pass back an String indentation rather than a count. Then we'd need to also change the indent that's written into a String, e.g. here: https://github.com/mitsuhiko/insta/blob/a07a24471e9e0621419e28062abf62f75e8b2aef/cargo-insta/src/inline.rs#L17

I don't think this is a personal priority for me nor a foundational feature for insta, so I won't get to it. But I'd be very happy to take a PR...

mitsuhiko commented 2 weeks ago

We should correctly use a string here since someone could mix tabs and spaces and we would want to emit a tab when a tab is used.