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

Use INSTA_UPDATE=force for forcing snapshot updates #482

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

max-sixty
Copy link
Sponsor Contributor

@max-sixty max-sixty commented May 2, 2024

In an effort to simplify the configs, this merges the INSTA_UPDATE and INSTA_FORCE_UPDATE configs. Conceptually, INSTA_FORCE_UPDATE overwrites INSTA_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 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 and this makes a step towards that.

It stacks on #479, which should merge first.

I'd be open to writing some tests for this if that'd be helpful.

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.
@mitsuhiko
Copy link
Owner

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.

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.

None yet

2 participants