Adding Diff Trimmed Lines #47
Conversation
I extended the LineDiff.tokenize function to add the ability to ignore leading and trailing spaces when using TrimmedLineDiff, and added diffTrimmedLines to the public-facing functions. My changes should not affect any previous functionality.
|
I've realized that I did not add documentation in the README, I can do this as well once this pull request is merged. |
|
@JamesGould123 you can push additional commits to the branch with any changes that you'd like to make. Docs and tests are something I'd like to see before merging. |
| newString = this.tokenize(newString); | ||
| oldString = this.tokenize(oldString); | ||
| newString = this.tokenize(newString, !!self.ignoreTrim); | ||
| oldString = this.tokenize(oldString, !!self.ignoreTrim); |
There was a problem hiding this comment.
Rather than changing the tokenize API, can't we just read this flag below?
There was a problem hiding this comment.
Oh, certainly, thanks for the line note. I worked myself into a logical corner, and couldn't think of any other way to do that when I was coding, so I did this hesitantly. Looking at it again, the correct solution is quite clear.
|
@kpdecker Sorry, I'm new to using Github, didn't know I could edit a pull request I've already sent, I'll add documentation and look into adding tests tonight. |
|
@JamesGould123 no worries. Welcome to github! :) Just so you are aware, github does not send updates when the PR changes, so you should make a comment here saying that you've made the changes so I'll get a notification. |
Refactoring to avoid changing tokenize API, adding documentation to
README.md, and adding 4 tests for TrimmedLineDiff to diffTest.js.
I also found a bug in how TrimmedLineDiff handled windows new lines
while adding the test, as well as changed TrimmedLineDiff so it doesn't
add a newline character ('\n') if it is the last line.
|
@kpdecker Thanks! Okay, I've overcome my fear of npm, added tests, moved the edits I made to the tokenize API into the LineDiff.tokenize function, and added documentation. |
| @@ -33,6 +33,10 @@ or | |||
|
|
|||
| Returns a list of change objects (See below). | |||
|
|
|||
| * `JsDiff.TrimmedLineDiff(oldStr, newStr[, callback])` - diffs two blocks of text, comparing line by line, ignoring leading and trailing whitespace. | |||
There was a problem hiding this comment.
This should be JsDiff.diffTrimmedLines
I updated the Readme with the Function rather than the Object name. In the windows new line merge, I added a check for '\n', as well as added a comment. The reason I've extended the windows new line merge functionality is to handle any situations where '\n\n' may come up. We want '\r\n' instead.
|
Okay, I just pushed a commit fixing the documentation issue and extending the merge lines functionality and adding a comment explaining it. |
|
@kpdecker Have you looked at the most recent changes? I've done everything you requested other than changing the test, which I explained and asked your opinion on above. |
| retLines[retLines.length - 1] += '\n'; | ||
| if (line === '\n' && lastLine && | ||
| (lastLineLastChar === '\r' || lastLineLastChar === '\n')) { | ||
| if(this.ignoreTrim || lastLineLastChar === '\n'){ |
There was a problem hiding this comment.
Please match whitespace style. (Space after if and before {, no else on the same line)
There was a problem hiding this comment.
I think you mean else should be on on the same line, it appears to be same line throughout the rest of the file, like this:
if () {
} else {
}
There was a problem hiding this comment.
There are several places in applyPatch where these rules are not followed, I'll fix that as well.
|
A side note, I'm leaving for vacation shortly so my responses might take a bit longer, but I do want to get this merged! |
Fixing everything discussed in the pull request. Also noticed the comment for the Trimmed Line Diff tests just said Line Diff, and fixed if ( style lower in diff.js
|
@kpdecker, okay I've fixed everything except that last issue. Have a good time on your vacation! |
|
Released in 1.3.0 |
|
Thanks 😄 |
I extended the LineDiff.tokenize function to add the ability to ignore leading and trailing spaces when using TrimmedLineDiff, and added diffTrimmedLines to the public-facing functions. My changes should not affect any previous functionality.