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

Raise an error on sufficiently different insta vs cargo-insta versions #509

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

Conversation

max-sixty
Copy link
Collaborator

@max-sixty max-sixty commented Jun 17, 2024

This has two purposes:

  • By raising an error if major versions differ, it allows eventually upgrading to insta 2 without confusing breaks (we want to add this a while before releasing v2)
  • It potentially allows for making some changes before a big break, by warning when minor versions differ sufficiently. This could be upgraded to an error. Currently we have a few deprecated items; and I was thinking of trying to unify some of the pending / named snapshot handling, but it becomes burdensome if we need to support the existing approach forever.
    • ...one advantage of gradual changes over a big break is that a big break would cause issues with cargo-insta — unless it worked with both insta 1.0 & 2.0, folks would need to be flipping between versions when working between projects that have vs. haven't upgraded. Whereas most projects can be expected to upgrade minor versions after a while. (For reference, 10 minor version is currently 15 months ago)

WDYT?

…sions

This has two purposes:
- By raising an error if major versions differ, it allows eventually upgrading to insta 2 without confusing breaks (we want to add this sufficiently before releasing v2)
- It potentially allows for making some changes before a big break, by warning when minor versions differ sufficiently. This could be upgraded to an error. Currently we have a few deprecated items; and I was thinking of trying to unify some of the pending / named snapshot handling, but it becomes burdensome if we need to support the existing approach forever.

WDYT?
@mitsuhiko
Copy link
Owner

But then this just says we break compatibility which means that someone would need to be able to run cargo insta1 and cargo insta2 if they need to support both things on a machine. That seems not ideal?

@max-sixty
Copy link
Collaborator Author

But then this just says we break compatibility which means that someone would need to be able to run cargo insta1 and cargo insta2 if they need to support both things on a machine. That seems not ideal?

Very much agree (some of what I meant in the bullet above starting "...one advantage of gradual changes over a big break "....).

Are you thinking that cargo-insta=2 would still support snapshots from cargo-insta=1? That would be a much better UX, at the cost of more work to support.


Even without the warning of v2 vs v1, this raises a warning on sufficiently different minor versions of insta vs. cargo-insta; is that worthwhile? My intention was for that to make it easier to very gradually make changes without folks getting confusing test failures.

@mitsuhiko
Copy link
Owner

I think that cargo-insta should definitely support old and new snapshots. My eventual goal at the moment is still to add snapshot!(...) macro but I don't want to deprecate all the existing macros for quite a while. So I fully expect that even if insta2 ends up having a dramatically simpler core crate, cargo-insta just keeps supporting old stuff. Even just to maybe help with migration etc.

I wonder if the right approach here might be to add a "snapshot version" marker to the snapshot files and to use that to set a lower bound to cargo-insta?

@max-sixty
Copy link
Collaborator Author

I think that cargo-insta should definitely support old and new snapshots. My eventual goal at the moment is still to add snapshot!(...) macro but I don't want to deprecate all the existing macros for quite a while. So I fully expect that even if insta2 ends up having a dramatically simpler core crate, cargo-insta just keeps supporting old stuff. Even just to maybe help with migration etc.

OK amazing

I wonder if the right approach here might be to add a "snapshot version" marker to the snapshot files and to use that to set a lower bound to cargo-insta?

I thought about this too! I think it's a nice legible approach.

The reason I leaned away is:

  • How would this update? Most snapshots are compatible for a long long time. So this would require rewriting snapshots to just change the version number every-so-often.
  • It wouldn't help for inline snapshots
  • We can generally tell from the snapshot whether it's old without needing a version number. For example, when we first had inline snapshots with leading characters, or the prior metadata format. I've recently added some warnings for old snapshots.

A couple of alternatives:

  • We could start increasing the major version of insta, even without doing big changes, and have a contract like "no warnings in the final minor version means the next major version will work". I think if we do this then we don't even need a version field, since the version field is the major version of insta in Cargo.toml.
  • A version in the insta config file (rather than each snapshot) avoiding the problems above.
    • When a snapshot was created with a version older than that version, it's not guaranteed to work — meaning that the failures would come when updating the config file, rather than when using a more recent version of insta
    • We could enforce something like "the minor version needs to be within 10 versions of the current insta version", which would allow us to stop supporting very old snapshots.
    • Relative to the major version changes, this approach arguably works better when there are incompatible changes in a very small number of snapshots. (It has nice aesthetics — it doesn't look like things are changing often to almost everyone...)

If it were just me, I would initially vote to start increasing the major version even without big changes. We could even do that soon: i.e. check we have warnings on everything that's old, confirm that --force-update-snapshots correctly rewrites old formats, check that cargo-insta would be OK on both old & new versions — and then bump the major version. Though probably worth waiting for some changes such as whitespace to land.

@mitsuhiko
Copy link
Owner

How would this update? Most snapshots are compatible for a long long time. So this would require rewriting snapshots to just change the version number every-so-often.

It would really only update in case an insta-2 comes around I believe. So we would basically be on version 2 for now (since we did one change in the past) until we finally decide to really change things around.

The global version in a config would also be an option but I would need to think about this more.

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.

2 participants