-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Conversation
#8982 moved the indicators to precede the counts instead of follow them:
EDITED: improvement is at right of annotation lines Example of insert in default export with ordinary matcher colors: Example of change in Example of insert and change in default export with possible future snapshot colors: |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool feature!
packages/jest-diff/README.md
Outdated
``` | ||
|
||
```diff | ||
- Expected 1 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That question lead me to discover that
Is it helpful (and not too late) to change the options from |
Should be good to change - 25 isn't stable yet |
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. |
Summary
Knowing the number of change lines can help interpret a diff, for example:
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