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

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

Merged
merged 7 commits into from Jun 22, 2020

Conversation

michael-brade
Copy link
Contributor

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
Copy link

jsf-clabot commented May 13, 2020

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented May 13, 2020

Coverage Status

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

@michael-brade
Copy link
Contributor Author

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.

@michael-brade michael-brade marked this pull request as draft May 13, 2020 09:42
the color() function is also used by the summary etc.
@Munter
Copy link
Member

Munter commented May 13, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

before:
image

after:
image

@michael-brade michael-brade marked this pull request as ready for review May 13, 2020 12:58
@Munter
Copy link
Member

Munter commented May 13, 2020

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:

image

@michael-brade
Copy link
Contributor Author

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
Copy link
Member

Munter commented May 13, 2020

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
Copy link
Contributor Author

This is what it would look like instead:
image

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

@michael-brade
Copy link
Contributor Author

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

@michael-brade
Copy link
Contributor Author

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

Anyway, now it's up to you 😁

@boneskull
Copy link
Member

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

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

@michael-brade
Copy link
Contributor Author

Sure. Here:
image

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

@craigtaub
Copy link
Contributor

craigtaub commented May 16, 2020

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.

Copy link
Member

@Munter Munter left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

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 boneskull added the status: waiting for author waiting on response from OP - more information needed label Jun 8, 2020
@boneskull
Copy link
Member

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
Copy link
Contributor Author

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

Copy link
Member

@outsideris outsideris left a comment

Choose a reason for hiding this comment

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

Linting is broken in base.js.

@michael-brade
Copy link
Contributor Author

Fixed.

Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@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 Outdated Show resolved Hide resolved
@juergba 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 juergba added this to the next milestone Jun 18, 2020
@juergba 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
Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@michael-brade thank you

@juergba juergba merged commit a1d3984 into mochajs:master Jun 22, 2020
@michael-brade
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: usability concerning user experience or interface semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants