Sort out behaviour of newlineIsToken and ignoreWhitespace #486
Conversation
|
@Mingun I'd welcome any comments you have on this. Have I failed to properly handle anything here that your PR did better? |
| it( | ||
| 'should not consider adding whitespace to an empty line an insertion ' + | ||
| 'even in newlineIsToken mode where a token may be an empty string', | ||
| function() { | ||
| const diffResult = diffTrimmedLines('foo\n\nbar', 'foo\n \nbar', {newlineIsToken: true}); | ||
| expect(convertChangesToXML(diffResult)).to.equal('foo\n \nbar'); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Ah, crap, I meant to remove this test before merging. It doesn't pass (and I added docs noting that!)
|
My intention was to get correct diff behavior that you usually expect from code diffs. When I implemented my fix in #219 I think, I got the expected results, that TortoiseGitMerge and similar tools give you when you merge file in ignored whitespace mode. I probably didn't actually investigate that behavior is identical, but I remember that at least results was not surprising:
Anyway, I think, that your fix is simpler, but I suggest to take tests from my PR, because they:
I think, that having them it is much better to understand what to expect from results. |
Three fixes here:
ignoreWhitespacenow behaves the same as otherignoreXxxmethods - i.e. changes how tokens are compared for equality but does NOT affect thevalues that show up in the returned change objects. Resolves FixdiffTrimmedLinesnot keep whitespaces in the output #219.newlineIsTokenandignoreWhitespace. Resolves Document (and throw an error over) the incompatibility between newlineIsToken and ignoreWhitespace #472, albeit not quite in the way I'd originally intended.diffTrimmedLines. I don't see a reason to have two ways (diffTrimmedLines&ignoreWhitespace) to do exactly the same thing, so I've purged one from the documented API. (I'll still keep the method itself around just for backwards compat, though.)For me to do before merging:
diffTrimmedLines.diffTrimmedLinesnot keep whitespaces in the output #219. (I didn't read the diff there before doing my own work on this - I'd actually forgotten @Mingun had actually put up a PR about this and not just an issue.)newlineIsTokenandignoreWhitespace.