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

Numerous needless hashes are wrapping the snapshot string #600

Open
nyurik opened this issue Sep 19, 2024 · 3 comments
Open

Numerous needless hashes are wrapping the snapshot string #600

nyurik opened this issue Sep 19, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@nyurik
Copy link

nyurik commented Sep 19, 2024

What happened?

If a string contains an character that requires escaping in a non-raw literal, the number of hashes is incorrect. This is especially noticeable when validating markdown generation.

Reproduction steps

// No newlines, a single `\` (escaped)
let value = "abc \\######### xyz";
assert_snapshot!(value, @r##########"abc \######### xyz"##########);

// Same number of hashes, only this time there is no escapable symbols
let value = "abc ######### xyz";
assert_snapshot!(value, @"abc ######### xyz");

// A single newline
let value = "abc \n######### xyz";
assert_snapshot!(value, @r##########"
abc 
######### xyz
"##########);

// Copy of the previous test - note that newline is inserted before and after, plus indentation. I assume this is by design?
let value = "
abc
######### xyz
";
assert_snapshot!(value, @r##########"

    abc
    ######### xyz
    
"##########);

Insta Version

1.40.0

rustc Version

1.81.0

What did you expect?

There should be as many hash characters as really needed.

@nyurik nyurik added the bug Something isn't working label Sep 19, 2024
@max-sixty
Copy link
Sponsor Collaborator

Re:

// Copy of the previous test - note that newline is inserted before and after, plus indentation. I assume this is by design?

...this should be solved by #581 (and tested / supplemented by #563). These didn't get a review before the most recent release.


Re the hashes: we previously always used ###, this was recently changed to base off the number of hashes in the string. There was a preference to always use raw strings (#561 (comment)) — but we can rethink how we decide the number of hashes.

What rule would you propose?

@nyurik
Copy link
Author

nyurik commented Sep 19, 2024

thx for the quick and thorough reply!

I think we should base it off of the definition of the raw string itself -- it ends when it sees a "#### - so we just need to find the longest double quote+hashes string, and add one more hash to use as the string declaration.

regex dependency is currently optional, but could be made required, in which case something like this:

    let text = "foo ### bar";
    let re = Regex::new("\"#*").unwrap();

    let number_of_hashes_to_use = re
        .find_iter(text)
        .map(|m| m.as_str().len())
        .max()
        .unwrap_or(0);

@max-sixty
Copy link
Sponsor Collaborator

Yes I think that could work quite well!

We are really vigilant about dependencies but could do something similar without regex.

No pressure but if you wanted to do a PR that would be great; otherwise I'll try and get to it soom.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants