kpdecker / jsdiff

A javascript text differencing implementation.
BSD 3-Clause "New" or "Revised" License
7.69k stars 491 forks source link

Add further assertion to test, as suggested by Mingun #491

Closed ExplodingCabbage closed 4 months ago

ExplodingCabbage commented 4 months ago

When I added this test, I decided to modify the assertion it makes to be easier to understand and verify the correctness of at a glance, at the expense of making it a less robust test of the behaviour. @Mingun indicated his disagreement with this at https://github.com/kpdecker/jsdiff/pull/488#discussion_r1491619027 and I immediately agreed he had a point, so I've added the more rigorous assertion back in alongside the readable one.

Interestingly, just directly copying @Mingun's assertion failed; @Mingun's version ends with:

        '<del>]</del><ins>  ]\n',
        '}</ins>'

but with my version of the ignoreWhitespace code, the line with the ] on it isn't treated as deleted and re-inserted at all, simply as equal/preserved. I guess this is a difference in how @Mingun's logic from https://github.com/kpdecker/jsdiff/pull/219 and mine from https://github.com/kpdecker/jsdiff/pull/486 handle the case where in one text a line is the final line and doesn't have a trailing newline character. I prefer the way my code seems to handle this (i.e. ignoring the addition of a trailing newline to a line that didn't have it), so I'm glad we've got a test for it now!

Thanks again to @Mingun for nudging me into doing things better. :smile:

Mingun commented 4 months ago

I prefer the way my code seems to handle this (i.e. ignoring the addition of a trailing newline to a line that didn't have it), so I'm glad we've got a test for it now!

Yes, I also agree with that.