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

Align inine snapshot handling with file snapshots #510

Open
max-sixty opened this issue Jun 17, 2024 · 1 comment
Open

Align inine snapshot handling with file snapshots #510

max-sixty opened this issue Jun 17, 2024 · 1 comment

Comments

@max-sixty
Copy link
Sponsor Collaborator

max-sixty commented Jun 17, 2024

I'm finding the crate is getting increasingly complicated, to the extent that as the second contributor (albeit far behind @mitsuhiko...) I'm sometimes scratching my head in how things work. Features such as #489 are great but will continue to make the main path more complicated.

Potentially some unnecessary complication comes from how differently inline & file snapshots are handled:

  • Pending inline snapshots write to .pending-snap files, "pending" file snapshots write to .snap.new files
  • Pending inline snapshots contain both the old & new value; pending file snapshots only contain the new value
  • Pending inline snapshots have a single file per rust file in which each snapshot gets a separate json entry, pending file snapshots have a file per snapshot
  • Pending inline snapshots track a run-id

Could we instead use .snap.new files for both file and pending snapshots, with a marker in their metadata indicating which is which? We'd read the old value from the rust file itself, wouldn't need to track run-id, and so collapse a lot of code in our main path. (We'd need to think about how line numbers change as code is updated, maybe other things I haven't thought of). As well as making the crate simpler, we also make its explanation simpler — when writing docs we often have to give footnotes like this:

insta/insta/src/lib.rs

Lines 171 to 172 in 58daea1

//! value into a draft file (note that inline snapshots use `.pending-snap`
//! files rather than `.snap.new` files). Running `cargo insta review` will
because of the way things are handled.

We'd need to handle both for a while, and so somewhat dependent on a mechanism such as #509 to ensure we don't have to handle both forever

@mitsuhiko
Copy link
Owner

I agree on the complexity. I'm really mentally going more and more back to the discussion in #456. I think that part of the challenge here really is that these two features (inline and non inline) just came at different times and in the wrong order.

If you treat the primary goal of the crate to just update snapshots in locations, then how that happens can be simplified. Say we have a snapshot!(...) macro then all it does, is recording changes and either applying them instantly or deferred. The reason the .pending stuff exists for inline snapshots has been concurrency problems with the way cargo tests sometimes run and that it's impossible to detect the end of a run. However I believe that there might be better ways to go around this.

But I agree that this should be aligned between the two modes.

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

No branches or pull requests

2 participants