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

Adjust force-update on inline snapshots to only view within the string #581

Merged
merged 15 commits into from
Sep 19, 2024

Conversation

max-sixty
Copy link
Collaborator

@max-sixty max-sixty commented Sep 1, 2024

This solves the issue in #573 for the moment:

  • When --force-update-snapshots in passed, we only update inline snapshots when there's some difference within the string, such as an additional linebreak at the start or the end
  • We no longer update the surrounding hashes. In the existing code, this happened because we were writing too many inline snapshots, not because we were actually checking the number of hashes
  • The main change in the code is that we store the unnormalized snapshot and then normalize when we need to. The existing code stored the normalized version, which meant we couldn't evaluate whether the unnormalized versions were different

It's a decent number of changes, but will integrate nicely with #563.

(FYI we currently don't look at the indentation, but we could adjust this) Now indentation works too

@max-sixty
Copy link
Collaborator Author

max-sixty commented Sep 2, 2024

OK, I think this is in a pretty good state now — again, more changes than ideal — but I think moving the library to be simpler, fewer corners

(+ with #563 we'll separate out any legacy snapshot normalizations, will be good)

@max-sixty
Copy link
Collaborator Author

@mitsuhiko this one is top of my list for you to review when you get the chance to review things; think it's a much better experience for users; somewhat to your point when merging the PR that enabled forcing inline snapshot updates

@max-sixty
Copy link
Collaborator Author

@mitsuhiko I think the benefits of this are large enough, and I would like to minimize conflicts with #489, that it's worth me merging without waiting longer for a review. The main drawback is potentially one of performance — the current code does many more allocations, albeit in non-perf-sensitive code.

I recognize you gave me merge permissions with the understanding I would be careful, so:

  • For the future, was merging this OK or would you have preferred I wait?
  • I'll address any objections
  • If you strongly object, I'll own reverting and rectifying any conflicts

@max-sixty max-sixty merged commit 5c6ba53 into mitsuhiko:master Sep 19, 2024
15 checks passed
@max-sixty max-sixty deleted the force-update-inline-no-hashes branch September 19, 2024 16:01
max-sixty added a commit that referenced this pull request Sep 19, 2024
Stacks on #581, merge that first

Now that we've consolidated the checks for whether to write snapshots
into `matches_fully`, I think we can remove the check on whether the
file contents match.
max-sixty pushed a commit that referenced this pull request Oct 7, 2024
As discussed in #610 it probably makes sense to be explicit about the
`internals` module not being guaranteed to be stable.

I also noticed that `TextSnapshotKind` gets exposed globally. This was
added in #581 when it was still called `SnapshotKind`. I wonder if this
is necessary to expose toplevel.
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.

1 participant