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 includeChangeCounts and rename Indicator options #8881

Merged
merged 5 commits into from
Aug 29, 2019

Conversation

pedrottimark
Copy link
Contributor

Summary

Knowing the number of change lines can help interpret a diff, for example:

  • only delete lines
  • only insert lines
  • equal number of delete and insert lines
  • almost equal number of delete and insert lines
  • quite unequal number of delete and insert lines

Question for separate pull request: include change counts in report when Jest matcher fails?

Test plan

Updated 18 and added 6 tests to diff.test.ts

See also pictures in the following comment

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Aug 27, 2019

#8982 moved the indicators to precede the counts instead of follow them:

- Expected  - 0
+ Received  + 3

EDITED: improvement is at right of annotation lines

Example of insert in default export with ordinary matcher colors:

includeChangeCounts 1c

Example of change in diffStringsUnified with current snapshot colors:

includeChangeCounts 2c

Example of insert and change in default export with possible future snapshot colors:

includeChangeCounts 3c

@codecov-io
Copy link

codecov-io commented Aug 27, 2019

Codecov Report

Merging #8881 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #8881     +/-   ##
=========================================
+ Coverage   64.11%   64.21%   +0.1%     
=========================================
  Files         275      275             
  Lines       11623    11657     +34     
  Branches     2846     2847      +1     
=========================================
+ Hits         7452     7486     +34     
  Misses       3545     3545             
  Partials      626      626
Impacted Files Coverage Δ
packages/jest-diff/src/normalizeDiffOptions.ts 100% <ø> (ø) ⬆️
packages/jest-diff/src/diffLines.ts 97.12% <100%> (+0.19%) ⬆️
packages/jest-diff/src/printDiffs.ts 96.47% <100%> (+1.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb7b132...364d640. Read the comment docs.

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.

Cool feature!

```

```diff
- Expected 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered the format

-1  Expected
+2  Received

? It is perhaps less likely to be mistaken for the total line counts of Expected/Received if the number is close together with the +/-.
Maybe there's prior art in other diff renderers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have a gift for identifying realistic first impressions!

Let’s explore alternatives. An earlier prototype was to repeat the + and - something like:

- Expected  -1
+ Received  +2

but the option for alternative symbols like < or > made me think twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is your interpretation if you see a ratio?

- Expected  1 / 2
+ Received  2 / 3

Copy link
Contributor

Choose a reason for hiding this comment

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

What is your interpretation if you see a ratio?

Not very clear to me, esp. since the sign is still far from the number.

Brainstorming things to show both number (not final):

5 Expected (-1)
6 Received (+2)

But I'm not sure if showing the totals is a good idea at all, if you just look at 5,6 it looks like just one line differs when it really could be 11.

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 agree that totals were unclear. Here a line from git diff --stat

1 file changed, 91 insertions(+), 22 deletions(-)

It suggested symbols following counts. For example, with two sets of options:

includeChangeCounts 1a

Let’s keep the labels aligned with the first column of compared content

@pedrottimark
Copy link
Contributor Author

Maybe there's prior art in other diff renderers?

That question lead me to discover that git diff uses the term indicator:

--output-indicator-new=<char>
--output-indicator-old=<char>
--output-indicator-context=<char>
Specify the character used to indicate new, old or context lines in the generated patch.
Normally they are +, - and ' ' respectively.

Is it helpful (and not too late) to change the options from aSymbol to aIndicator and so on?

@SimenB
Copy link
Member

SimenB commented Aug 29, 2019

Is it helpful (and not too late) to change the options from aSymbol to aIndicator and so on?

Should be good to change - 25 isn't stable yet

@pedrottimark pedrottimark changed the title jest-diff: Add includeChangeCounts option jest-diff: Add includeChangeCounts and rename Indicator options Aug 29, 2019
@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.

5 participants