add unified diff separator #2846
Conversation
|
|
|
I know the label said 'semver major' - but I didn't see a major branch to point the PR to. I figured master was the correct one, but please let me know if that's incorrect. |
There was a problem hiding this comment.
Thanks for the implementation! And especially for providing a test with it!
There's one thing about the test that really needs to be updated for full correctness; after that, we'll just have to figured out which branch we want to put semver-major changes in...
|
|
||
| regexesToMatch.forEach(function (aRegex) { | ||
| errOut.should.match(aRegex); | ||
| }); |
There was a problem hiding this comment.
Instead of matching each line's regex against the whole output string, which does not guarantee order as far as I am aware, let's either match a single regex covering multiple lines, or else use .forEach(function (aRegex, index) { <next line> errOut.split('\n')[index].should... instead of .forEach(function (aRegex) { <next line> errOut.should...
There was a problem hiding this comment.
When I PR - I try to avoid corrections like these and just stick with the patterns in the existing codebase. I too thought this pattern was weird, but it's what's in the code.
If you really want me to make it different for this particular test I will, but I suggest refactoring them all in a separate PR for more accurate test coverage.
There was a problem hiding this comment.
Ouch! My bad; I hadn't noticed those tests before.
In that case I'd be fine merging this as-is for consistency and then doing a separate thorough sweep to correct all instances of that pattern.
Thanks for identifying that!
lib/reporters/base.js
Outdated
| @@ -393,7 +393,7 @@ function inlineDiff (err, escape) { | |||
| */ | |||
| function unifiedDiff (err, escape) { | |||
| var indent = ' '; | |||
| function cleanUp (line) { | |||
| function cleanUp (line, idx) { | |||
There was a problem hiding this comment.
The idx added here does not appear to be used?
There was a problem hiding this comment.
Whoops - sorry about this. An artifact of my debugging.
ScottFreeCode
added
the
semver-major
- fix issue mochajs#2295 - the purpose of this feature is to make the unified diff format more readable. Without it, different portions of the same actual/expected diff look contiguous even though they are actually separated by the default context of four lines.
olsonpm
force-pushed
the
feature/unified-diff-separator
branch
from
8cf2222
to
26aaf51
Compare
|
@ScottFreeCode - Just pushed the removal of the unused |
|
i appreciate the time you spent with this. i know it wasn't a high priority nor highly visible issue. |
- fix issue mochajs#2295 - the purpose of this feature is to make the unified diff format more readable. Without it, different portions of the same actual/expected diff look contiguous even though they are actually separated by the default context of four lines.
format more readable. Without it, different portions
of the same actual/expected diff look contiguous even
though they are actually separated by the default
context of four lines.