Don't allow missing snapshots without --snapshot-update

[davidshepherd7]

Fixes #73

Possibly should only be included in a new major version so that it doesn't break people's CI.

[dashmug]

Why not add a command line flag to enable it (disabled by default) so you can ship this in a minor version?

Then, you can have that flag enabled by default in the next major version.

[davidshepherd7]

Yeah, I suspect something like that is the way to do it. Unfortunately this repository seems to be unmaintained.

[davidshepherd7]

@paulmelnikow Do you think it's worth adding a command line flag to control this behaviour?

I feel like this should be the default behaviour ASAP because the old version could cause spurious test passes. So I think it would be reasonable to release a new major version now-ish with no breaking changes other than this. If we do that then I think there's no need to add a flag and immediately deprecate it because anyone who wants to opt-in to the new behaviour can upgrade and anyone who doesn't want it right now can not upgrade.

[paulmelnikow]

I feel strongly that this the current behavior is incorrect and dangerous. Until someone articulates an important use case for the flag (that can't be worked around easily) I think we should not provide a flag.

I think this is a bug fix, but agreed it is a major enough change in behavior that it should be called a breaking change.

I feel a lot of urgency about fixing this too; just want to do so with care 😀 Once we get it in good shape I think we can merge it and set the version number accordingly.

[paulmelnikow]

I'm a bit confused about why should_update_snapshot is used in some places and snapshot_should_update in others. I think this probably needs a closer look before it's merged.

[davidshepherd7]

Oh yeah, I didn't notice that there was a snapshot_should_update on some types of test case already. I've changed it to use that now.

I also rebased onto the black commit by running black on my own branch, squashing my branch, and rebasing (merging normally was a complete mess). I think it's done something sensible.

[medmunds]

(Also closes #25, I think)

[medmunds]

rebased onto the black commit

tests/test_formatter.py and tests/test_sorted_dict.py seem to have been reformatted without any other changes, and should probably be omitted from this PR.

Other than that, looks good to me. (And much easier to follow the should_update logic across the different frameworks now!)

[skovhus]

@paulmelnikow @davidshepherd7 it would be awesome to get this merged. Anything I can do to help here?

[davidshepherd7]

IIRC the code is all ready to go and it just needs someone to merge it.

On Thu, 13 May 2021, 19:44 Kenneth Skovhus, @.***> wrote:

@paulmelnikow https://github.com/paulmelnikow @davidshepherd7 https://github.com/davidshepherd7 it would be awesome to get this merged. Anything I can do to help here?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/syrusakbary/snapshottest/pull/112#issuecomment-840755944, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGCJVFDVW2M5SQAOSOGMFDTNQMX5ANCNFSM4KVX43FA .

[skovhus]

It would make sense to merge and just release as part of the 1.0 beta.

[jackton1]

Where are we on merging this ??

Hit this issue a couple of times.

[j0nm1]

Any updates here? This is a major problem for bigger projects.