search

concordancejs / concordance

Compare, format, diff and serialize any JavaScript value
ISC License
207 stars 15 forks source link

Invert string diffs #69

Closed ninevra closed 3 years ago

ninevra commented 3 years ago

Fixes #40 by passing the invert option through into diffDeep().

When invert is set, StringValue.prototype.diffDeep() switches theme.string.diff.insert for theme.string.diff.delete, theme.string.diff.insertLine for theme.string.deleteLine, and vice versa.

PrimitiveItem.protototype.diffDeep(), MapEntry.prototype.diffDeep(), and PrimitiveProperty.prototype.diffDeep() pass the invert option on to the values they wrap, enabling nested string diffs to invert properly.

The diff.js.md diff is unfortunately messy, because it's been resorted. The changed snapshots are in inverted diffs, inverts string diffs, and inverts string diffs in containers. (One way to prove that would be for a collaborator to push a commit resorting all the snapshots, after which I could rebase this PR to get a cleaner diff.)

IssueHunt Summary ### Referenced issues This pull request has been submitted to: - [#40: String diffs don't invert properly](https://issuehunt.io/repos/87210037/issues/40) ---
ninevra commented 3 years ago

Per semver, this seems like a patch change, in that it fixes a bug and doesn't modify the public API. I don't think anyone should be relying programmatically on the exact details of AVA's diff output. On the other hand, it could break AVA's own reporter tests, if they use snapshot assertions and don't strip out the ansi color escape sequences. Potentially concordance's other users could find their tests breaking, too, if they use the invert option and have comprehensive tests.

If it were me, I'd release this as a patch version, update AVA's minimum concordance version, and fix its reporter tests if necessary. I can definitely see the rationale for releasing this as a breaking change just to be safe, but if fixing a bug that affects only a portion of the behavior of a likely-rarely-used option isn't non-breaking, then I'm not sure what is.

Ultimately it's a question of how exactly concordance's reverse-dependencies use it, though, and I'm not qualified to answer that.

novemberborn commented 3 years ago

Per semver, this seems like a patch change, in that it fixes a bug and doesn't modify the public API.

Agreed.

On the other hand, it could break AVA's own reporter tests, if they use snapshot assertions and don't strip out the ansi color escape sequences.

I don't think they do.

update AVA's minimum concordance version

Fresh installs will pull this in anyhow. And shipping a patch release in AVA is a bit trickier now. Let's wait and see if our integration tests fail.