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

Incorrect inline snapshot indentation with hard_tabs = true #679

Open
nitsky opened this issue Oct 31, 2024 · 4 comments
Open

Incorrect inline snapshot indentation with hard_tabs = true #679

nitsky opened this issue Oct 31, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@nitsky
Copy link

nitsky commented Oct 31, 2024

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.

@nitsky nitsky added the bug Something isn't working label Oct 31, 2024
@max-sixty
Copy link
Collaborator

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...

@max-sixty max-sixty removed the bug Something isn't working label Oct 31, 2024
@nitsky
Copy link
Author

nitsky commented Oct 31, 2024

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
Copy link
Collaborator

Ah sorry for indeed misunderstanding.

The root of this is here:

insta/insta/src/snapshot.rs

Lines 632 to 649 in ce0027a

fn count_leading_spaces(value: &str) -> usize {
value.chars().take_while(|x| x.is_whitespace()).count()
}
fn min_indentation(snapshot: &str) -> usize {
let lines = snapshot.trim_end().lines();
if lines.clone().count() <= 1 {
// not a multi-line string
return 0;
}
lines
.filter(|l| !l.is_empty())
.map(count_leading_spaces)
.min()
.unwrap_or(0)
}

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:
    indentation: usize,

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...

@max-sixty max-sixty added the enhancement New feature or request label Oct 31, 2024
@mitsuhiko
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants