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

jest-diff: Add options for colors and symbols #8841

Merged
merged 19 commits into from
Aug 22, 2019

Conversation

pedrottimark
Copy link
Contributor

@pedrottimark pedrottimark commented Aug 16, 2019

Summary

Fixes #7980

Let dependents configure jest-diff to follow other conventions, like diff or git diff

Add properties to DiffOptions type:

name default
aColor chalk.green
aSymbol -> aIndicator '-'
bColor chalk.red
bSymbol -> bIndicator '+'
commonColor chalk.dim
commonSymbol -> commonIndicator ' '
omitAnnotationLines false

EDIT: renamed Symbol as Indicator similar to git diff options in #8881

  • Remove named export getStringDiff and move conditions to callers of the following:
  • Add named export diffStringsUnified /cc @ewanharris does this fit your application?
  • Add named export diffStringsRaw
  • Add condition Number.isSafeInteger(contextLines) to fix TypeError: Cannot read property 'length' of undefined
  • Add README.md file for package :)

To help you prioritize your attention, the most complicated code changes:

  1. printDiffs.ts in jest-diff replace old export that was not re-usable outside of Jest :(
  2. index.ts in jest-matcher-utils include specific conditions on the calling side
  3. print.ts in jest-snapshot include specific conditions on the calling side

Part of me wishes this was in 24.9.0 and part of me is thankful that it isn’t

The next step will be to use these options for alternative colors in snapshot diffs

Test plan

  • Added 21 tests and renamed 1 test in diff.test.ts in jest-diff
  • Added 2 tests in printDiffOrStringify.test.ts in jest-snapshot
  • Added 4 tests in printDiffOrStringified.test.ts in jest-snapshot

Some new tests that I temporarily omitted from jest-diff confirmed a problem with substring highlight that I had seen recently while reviewing snapshot tests. I will fix it in a separate PR.

See also pictures in the following comment

@pedrottimark
Copy link
Contributor Author

Here is the realistic example of codemod from the issue by Ewan Harris

const options = {
  aAnnotation: 'Original',
  aColor: chalk.red,
  bAnnotation: 'Modified',
  bColor: chalk.green,
});
const difference = diffStringsAligned(a, b, options);

First example picture baseline above and optional below

codemod

2 more example pictures baseline at left and optional at right

const options = {
  aSymbol: '<',
  bSymbol: '>',
};
const difference = diffLines(a, b, options); // default export

change-symbols

const options = {
  commonColor: line => line,
  commonSymbol: '=',
};
const difference = diffLines(a, b, options); // default export

common

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Code / tests also appear solid after scrolling through the first time

@@ -160,6 +162,7 @@ namespace diff {
export type DiffOptions = JestDiffOptions;
}

diff.getStringDiff = getStringDiff;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking, correct? Just mentioning because the changelog entry does not say so

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had hoped to beat the minor because that export is only for first time in 24.9.0 :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's just pretend it was never public api. Nobody will be depending on it yet.

We might also not have a release until 25, at which point it doesn't matter anyways


Two named exports compare **strings** character-by-character:

- `diffStringsAligned` returns a string which includes comparison lines.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me quite a while (even after reading the whole readme) to understand the difference between those two. I still do not quite understand the naming, which I think was part of what confused me because I associated things like the beginning of lines with "aligned".
Would you say that unified diff vs split diff (like for example in GitHub) is a good analogy for those functions and could be used for naming?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on being a bit confused about what aligned means. Unified vs split makes more sense to me (that's also the naming bitbucket uses, so I think it's pretty standard)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant analogy!

  • so-called aligned is like unified diff
  • so-called unaligned is used in Jest to display one-line strings in normal compact report:
Expected: "change from"
Received: "change to"

For multiline strings, a split like GitHub would consist of array of string tuples:

[
  [a0, b0],
  [a1, b1],
  // and so on
]

for corresponding changed chunks. For deleted b is empty, for inserted a is empty.

I am curious, can y’all think of a use case in third-party dependents?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I have renamed diffStringsAligned and diffStringsUnified
  • I think that I will replace diffStringsUnaligned with diffStringsRaw that returns an array of Diff objects, so that it will be reusable to the max

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So happy this is getting a proper readme!

packages/jest-diff/README.md Outdated Show resolved Hide resolved
packages/jest-diff/README.md Show resolved Hide resolved

To use `diffLines` as the function name, write either of the following:

- `const diffLines = require('jest-diff');` in a CommonJS module
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll probably go for named exports in jest 25 (we wanna avoid having to use export =), but this is of course fine for now

@SimenB
Copy link
Member

SimenB commented Aug 18, 2019

Sorry about the conflict. It's an autifxable eslint rule, so shouldn't be too bad to resolve at least 🙂

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome, thank you so much for tackling it! I think this makes jest-diff pretty general purpose

@SimenB
Copy link
Member

SimenB commented Aug 22, 2019

@pedrottimark fix conflicts and merge this? I think it's good to go 🙂

@pedrottimark pedrottimark deleted the diff-options branch August 22, 2019 15:41
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow color and label options for jest-diff
4 participants