Skip to content

Conversation

@wetneb
Copy link
Contributor

@wetneb wetneb commented Nov 16, 2025

I made this update as part of updating ron in Debian and thought that you might like to have it upstream :)

@wetneb wetneb force-pushed the update_ron branch 2 times, most recently from c2162a2 to 57e0f45 Compare November 17, 2025 08:35
@max-sixty
Copy link
Collaborator

thanks for the contribution! could we try getting tests & the build passing?

@wetneb
Copy link
Contributor Author

wetneb commented Nov 17, 2025

Getting the build to pass sounds like an excellent idea :)

Rust was driving me slightly mad in this case, first by complaining about this "unused" import which then becomes "missing" once I remove it! Of course it's because I should have feature-gated it… Sorry about that!

@max-sixty
Copy link
Collaborator

first by complaining about this "unused" import which then becomes "missing" once I remove it

lol

@max-sixty
Copy link
Collaborator

merged; thank you @wetneb !

@max-sixty max-sixty merged commit dd34e41 into mitsuhiko:master Nov 17, 2025
15 checks passed
@tmke8
Copy link

tmke8 commented Nov 25, 2025

Huh, I thought insta was never going to update ron because it leads to different snapshots because the ron format changed? #414 (comment)

This did cause churn in my snapshots, though I personally don't really mind.

@wetneb
Copy link
Contributor Author

wetneb commented Nov 25, 2025

Sorry about that churn, I should have anticipated and highlighted that. I hope it's an acceptable inconvenience if it only happens rarely. Perhaps it would be worth adding a comment in Cargo.toml warning against bumping this dependency on a whim.

It would probably be worth highlighting the update in the release description, perhaps.

@max-sixty
Copy link
Collaborator

let's see whether we get any feedback. I admittedly didn't realize it would hit us, and we don't have the same level of testing for ron as we do for others.

I agree we likely can't stay on an old version forever, and the likelihood we get an insta 2.0 together soon is probably not that high

if we get a lot of issues, we could consider reverting / trying to handle both (but IIRC that's quite difficult...)

@ilyagr
Copy link

ilyagr commented Nov 27, 2025

Just for reference, here's what the most recent update of insta looked like for me:

jj-vcs/jj@2f0132a

I don't think the changes are ron-related (which makes it less bad to have ron-related or toml-related changes, in my view).

(This are the minimal changes to make tests pass again; --force-update-snapshots causes many more changes)

max-sixty added a commit to max-sixty/insta that referenced this pull request Nov 27, 2025
This fixes a regression introduced in 1.44.0 where existing inline
snapshots using the legacy multiline format would fail to match.

The problem: In 1.44.0, we changed how inline snapshots handle leading
newlines (PR mitsuhiko#563). This caused snapshots like:

    insta::assert_snapshot!(value, @r"
    Unconflicted Mode(FILE) 0839b2e9412b
    ");

to fail when the actual value is "Unconflicted Mode(FILE) 0839b2e9412b".
The legacy format stored single-line content in a multiline raw string
with source code indentation, which after processing becomes
"    Unconflicted...\n    " - the leading spaces from indentation
weren't being stripped in the comparison.

The fix: In `matches_legacy`, detect the legacy single-line-in-multiline
pattern by checking if the raw contents contain a newline but collapse
to a single line after trimming. When this pattern is detected, apply
`trim_start()` to strip the source code indentation artifacts.

This ensures:
- Legacy snapshots continue to pass (with a warning to update)
- Modern single-line snapshots with intentional spaces are preserved
- True multiline snapshots are unaffected

We take backward compatibility seriously - upgrading insta should not
cause existing valid tests to fail.

Reported-by: jj-vcs/jj
See: mitsuhiko#819 (comment)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
max-sixty added a commit that referenced this pull request Nov 27, 2025
## Summary

This fixes a regression introduced in 1.44.0 where existing inline
snapshots using the legacy multiline format would fail to match.

**The problem:** PR #563 changed how inline snapshots handle leading
newlines. This caused snapshots like:

```rust
insta::assert_snapshot!(value, @r"
    Unconflicted Mode(FILE) 0839b2e9412b
    ");
```

to fail when the actual value is `"Unconflicted Mode(FILE)
0839b2e9412b"`.

The legacy format stored single-line content in a multiline raw string
with source code indentation. After `from_inline_literal` processing,
this becomes `" Unconflicted...\n "` — the leading spaces from code
indentation weren't being stripped in the legacy comparison path.

**The fix:** In `matches_legacy`, detect the legacy
single-line-in-multiline pattern:

```rust
let is_legacy_single_line_in_multiline =
    sc.contents.contains('\n') && sc.contents.trim_end().lines().count() <= 1;
```

When this pattern is detected, apply `trim_start()` to strip the source
code indentation artifacts.

This ensures:
- ✅ Legacy snapshots continue to pass (with a warning to update)
- ✅ Modern single-line snapshots with intentional leading spaces are
preserved
- ✅ True multiline snapshots are unaffected

## Test plan

- [x] Added 8 comprehensive functional tests covering:
  - Single-line content in multiline format (the jj bug)
  - Raw string multiline variant
  - Force update reformatting works
  - True multiline content unaffected
  - Multiline with relative indentation preserved
  - Intentional leading spaces correctly fail (no false positives)
  - Trailing whitespace line edge case
  - Empty snapshot edge case
- [x] All 72 functional tests pass
- [x] Full test suite passes

Reported-by: jj-vcs/jj  
See: #819 (comment)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants