Use background color to show colored spaces in an inline diff #4287

michael-brade · 2020-05-13T09:06:23Z

Description of the Change

The inlineDiffs option is really nice, showing us in color the added/deleted parts. However, if only whitespace has changed, the diffs are useless because nothing is colored. So this little change replaces all whitespace with the unicode Open Box char.

Alternate Designs

n.a.

Why should this be in core?

The functionality already is in core.

Benefits

see above.

Possible Drawbacks

none.

Applicable issues

This is an enhancement.

jsf-clabot · 2020-05-13T09:06:24Z

All committers have signed the CLA.

coveralls · 2020-05-13T09:17:08Z

Coverage increased (+0.03%) to 93.795% when pulling 30ea7a0 on michael-brade:color-spaces into 9d4a8ec on mochajs:master.

michael-brade · 2020-05-13T09:41:50Z

Darn. Too early. This function is also used to color the summary. I will have to fix this pull request. Don't use it yet.

the color() function is also used by the summary etc.

Munter · 2020-05-13T11:56:50Z

From the description I like this change very much. Maybe you could add a screenshot to highlight the difference in output.

I do have some reservations though.

We are using https://www.npmjs.com/package/diff to create the diffing output. Might such an really belong in the diffing library instead?

What happens when I copy/paste the output from the diff? Will I get spaces or will I get \u2423 in my paste buffer? If I get \u2423 I think that would be a problem, as we are not correctly showing in the output what was actually diffed. Is there precedence for this in other libraries?

michael-brade · 2020-05-13T12:31:57Z

Good points. Regarding your first: no, I don't think so. mocha actually takes the diff and colors it after the external library has processed it. So the external lib should stay independent of this feature, it is solely on mocha's side.

Regarding copy&paste: yes, obviously you will get \u2423. I don't know any other library that does this, I was just annoyed because I work so much with spaces in LaTeX.js, so I need this and came up with this idea. However, the colored diff is unsuitable for copy&paste anyway because you get the old and new text mixed together. Above the colored diff you get AssertionError: expected '...' to equal '...' and that does include the original spaces. So I would say we are fine. Screenshot coming up soon.

michael-brade · 2020-05-13T12:58:42Z

before:

after:

Munter · 2020-05-13T13:28:56Z

I like the diffs from https://unexpected.js.org/assertions/string/to-be/ better, where they are using color inversion to turn the background colored. This highlights spaces very well:

michael-brade · 2020-05-13T14:30:00Z

Ah, interesting! I have wondered if I should do that instead but did not because it seems more like a break with the current way. However, if no one objects, I'll update the pull request.

Munter · 2020-05-13T14:41:56Z

It's certainly a change that people will notice. If it's for the better or worse I guess is up to interpretation. Should probably have @mochajs/core weigh in on this

michael-brade · 2020-05-13T14:45:20Z

This is what it would look like instead:

Let me know which version to use for the final PR.

michael-brade · 2020-05-13T14:49:14Z

Hm. However, this is indeed much more intrusive: when not using inlineDiffs, it looks like this:

michael-brade · 2020-05-13T14:54:12Z

It's a little bit better with black on green:

Anyway, now it's up to you 😁

boneskull · 2020-05-13T22:32:06Z

Hmm. Can you show black-on-red as well?

I think I like this idea better than using some random unicode character.

michael-brade · 2020-05-13T23:03:06Z

Sure. Here:

I have now updated the PR to use this last version.

craigtaub · 2020-05-16T11:04:29Z

I think the inversion produces a clearer separation for inline diffs in addition to the fix for whitespace.
My only issue is a slightly noisier terminal for normal diffs (as doesn't invert only the specific change unlike "unexpected.js", altho i'm aware this would be a new feature and not intention of this PR).
But overall I think an improvement.

Munter

I think this is an improvement as well. +1 for merging

juergba

If I have understood correctly this PR is about inlineDiff only, not the normal diff.
So why we don't leave the colors for the normal diff unchanged, and only change the colors for inlineDiff?

boneskull · 2020-06-08T23:48:06Z

I'm OK with it changing the colors for all diffs, but we should make sure that's the intent here (it does not seem to be, at least from the title). @michael-brade ?

michael-brade · 2020-06-09T21:26:01Z

Well, I don't mind either way. But it makes sense to change only the inline diff, so now I updated the code.

outsideris

Linting is broken in base.js.

michael-brade · 2020-06-14T22:40:59Z

Fixed.

juergba

@michael-brade I tested your changes, it really looks nice.
I have a small comment, then this PR should be ready to get merged.

lib/reporters/base.js

juergba

@michael-brade thank you

michael-brade · 2020-06-22T21:31:47Z

Sorry, I was in a hurry before, that's why I left no comment. Thanks for merging! :-)

use Open Box char to show colored spaces in an inline diff

b4d7825

michael-brade marked this pull request as draft May 13, 2020 09:42

don't change the color() function

926a4ed

the color() function is also used by the summary etc.

michael-brade marked this pull request as ready for review May 13, 2020 12:58

diffs: switch to using background color instead of visible spaces

a01b05e

Munter approved these changes May 19, 2020

View reviewed changes

juergba reviewed Jun 3, 2020

View reviewed changes

boneskull added the status: waiting for author waiting on response from OP - more information needed label Jun 8, 2020

only use background color for inline diffs

166ff91

michael-brade force-pushed the color-spaces branch from fc84532 to 166ff91 Compare June 9, 2020 21:24

outsideris suggested changes Jun 13, 2020

View reviewed changes

michael-brade added 2 commits June 14, 2020 23:52

Merge branch 'master' into color-spaces

ccad228

format

f75a2a4

juergba reviewed Jun 18, 2020

View reviewed changes

lib/reporters/base.js Outdated Show resolved Hide resolved

juergba added semver-patch implementation requires increase of "patch" version number; "bug fixes" area: usability concerning user experience or interface and removed status: waiting for author waiting on response from OP - more information needed labels Jun 18, 2020

juergba added this to the next milestone Jun 18, 2020

juergba requested review from outsideris and Munter June 18, 2020 06:43

juergba changed the title ~~use Open Box char to show colored spaces in an inline diff~~ Use background color to show colored spaces in an inline diff Jun 18, 2020

if you say so...

30ea7a0

juergba approved these changes Jun 22, 2020

View reviewed changes

juergba merged commit a1d3984 into mochajs:master Jun 22, 2020

michael-brade deleted the color-spaces branch June 22, 2020 21:31

Omrisnyk mentioned this pull request Dec 7, 2023

[Snyk] Fix for 1 vulnerabilities Omrisnyk/npm-lockfiles#131

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use background color to show colored spaces in an inline diff #4287

Use background color to show colored spaces in an inline diff #4287

michael-brade commented May 13, 2020

jsf-clabot commented May 13, 2020 •

edited

Loading

coveralls commented May 13, 2020 •

edited

Loading

michael-brade commented May 13, 2020

Munter commented May 13, 2020

michael-brade commented May 13, 2020

michael-brade commented May 13, 2020

Munter commented May 13, 2020

michael-brade commented May 13, 2020

Munter commented May 13, 2020

michael-brade commented May 13, 2020

michael-brade commented May 13, 2020

michael-brade commented May 13, 2020

boneskull commented May 13, 2020

michael-brade commented May 13, 2020

craigtaub commented May 16, 2020 •

edited

Loading

Munter left a comment

juergba left a comment

boneskull commented Jun 8, 2020

michael-brade commented Jun 9, 2020

outsideris left a comment

michael-brade commented Jun 14, 2020

juergba left a comment

juergba left a comment

michael-brade commented Jun 22, 2020

Use background color to show colored spaces in an inline diff #4287

Use background color to show colored spaces in an inline diff #4287

Conversation

michael-brade commented May 13, 2020

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

Applicable issues

jsf-clabot commented May 13, 2020 • edited Loading

coveralls commented May 13, 2020 • edited Loading

michael-brade commented May 13, 2020

Munter commented May 13, 2020

michael-brade commented May 13, 2020

michael-brade commented May 13, 2020

Munter commented May 13, 2020

michael-brade commented May 13, 2020

Munter commented May 13, 2020

michael-brade commented May 13, 2020

michael-brade commented May 13, 2020

michael-brade commented May 13, 2020

boneskull commented May 13, 2020

michael-brade commented May 13, 2020

craigtaub commented May 16, 2020 • edited Loading

Munter left a comment

Choose a reason for hiding this comment

juergba left a comment

Choose a reason for hiding this comment

boneskull commented Jun 8, 2020

michael-brade commented Jun 9, 2020

outsideris left a comment

Choose a reason for hiding this comment

michael-brade commented Jun 14, 2020

juergba left a comment

Choose a reason for hiding this comment

juergba left a comment

Choose a reason for hiding this comment

michael-brade commented Jun 22, 2020

jsf-clabot commented May 13, 2020 •

edited

Loading

coveralls commented May 13, 2020 •

edited

Loading

craigtaub commented May 16, 2020 •

edited

Loading