Use INSTA_UPDATE=force for forcing snapshot updates
#482
Conversation
This attempts to clarify when snapshots are written to draft `.snap.new` files as opposed to overwriting `.snap` files
In an effort to simplify the configs, this merges the `INSTA_UPDATE` and `INSTA_FORCE_UPDATE`. I don't think it's possible to require these both; they naturally fit into the same config setting. I realized after starting this that we want to be careful about supporting new & old versions of `cargo-insta`. So this takes a conservative approach, only changing `insta` at first, but with the future updates to `cargo-insta` commented in the code. I realize that adds a bit of complication; though on balance I think simplifying the configs would be helpful. It stacks on mitsuhiko#479, which should merge first. I'd be open to writing some tests for this if that'd be helpful.
|
I think this makes sense in general. I do want to run this first though so I will hold on merging until the next release. |
|
The latest commit:
|
We don't use this yet, but could be helpful in areas such as mitsuhiko#482
We don't use this yet, but could be helpful in areas such as #482
|
I would merge this but could you resolve the conflicts? |
|
OK, done. The prior commit actually caught a semantic conflict when running This PR ends up being a lot of additional code for a "simplification" PR. But in retrospect I think in the long-long term it does get simpler, and there's some nice machinery here for making |
|
Missed the release so changed the 1.39->1.40, 1.40->1.41 |
|
Will merge in the next week without more feedback, given the note above, to keep things moving. Lmk if that's too aggressive, both for this PR and the principle |
In an effort to simplify the configs, this merges the
INSTA_UPDATEandINSTA_FORCE_UPDATEconfigs. Conceptually,INSTA_FORCE_UPDATEoverwritesINSTA_UPDATE; they naturally fit into the same config setting.I realized after starting this that we want to be careful about supporting new & old versions of
cargo-insta. So this takes a conservative approach,only changingAdjusted to use the underlying version ofinstaat first, but with the future updates tocargo-instacommented in the code. I realize that adds a bit of complication; though on balance I think simplifying the configs would be helpful and this makes a step towards that.insta; I think a good approach!It stacks on #479, which should merge first.MergedI'd be open to writing some tests for this if that'd be helpful.Written