Split diffs based on file headers instead of 'Index:' metadata #88
Conversation
| while (i < diffstr.length) { | ||
| if (/^(Index:|diff -r|@@)/.test(diffstr[i])) { | ||
| // Parse diff metadata | ||
| for (; i < diffstr.length; i++) { |
There was a problem hiding this comment.
Not going to accept purely stylistic changes for fear of the noise impact. Show me the substance not your own style. When working in the OSS world you have to remember that you aren't coding for yourself unless it's your own project.
There was a problem hiding this comment.
This is not purely stylistic, this is to mimic the same style done on line 78 on this same file to have an uniform style on both places.
There was a problem hiding this comment.
"This is to mimic style" means it's stylistic. I'm glad to discuss stylistic changes in another PR if you want to spend your time on that but I don't accept them for functional changes.
There was a problem hiding this comment.
To that end style is anything that has the same effective functionality but just has different actual code.
The reason for this is that the functional changes are the important part and they should be focused on in isolation.
There was a problem hiding this comment.
Ok, removed style changes.
|
Still has some noise from the In the other PR you reference unified diff format, did you base this off of a spec? Could you point to where you got this from? It's still not clear to me from the code what the changes are here other than making things a bit less accepting by adding additional regex restrictions. I think the best way for you to express this is to write a test case that shows what would have failed before and how it doesn't now. |
http://www.gnu.org/software/diffutils/manual/html_node/Detailed-Unified.html As you can see there, the only important lines are the ones of the file header (+++ and ---), the beginning of the hunks (@@) and the description of the added and removed ones (+ and -). Any of the other lines (metadata and context) are purely optional.
Trying to use a diff file with no Index line doesn't work with |
|
Any update on this? |
| @@ -51,7 +50,7 @@ export function parsePatch(uniDiff, options = {}) { | |||
| // Parses the --- and +++ headers, if none are found, no lines | |||
| // are consumed. | |||
| function parseFileHeader(index) { | |||
| let fileHeader = (/^(\-\-\-|\+\+\+)\s(\S+)\s?(.*)/.exec(diffstr[i])); | |||
| let fileHeader = /^(\-\-\-|\+\+\+)\s+(\S+)\s?(.+)/.exec(diffstr[i]); | |||
There was a problem hiding this comment.
Your parenthesis change here introduces a linter error.
There was a problem hiding this comment.
You are right, I missed it. Done.
|
Changes uploaded |
| parsePatch('Index: foo\nfoo', {strict: true}); | ||
| }).to['throw'](/Unknown line 2 "foo"/); | ||
| parsePatch('Index: foo\n+++ bar\nblah', {strict: true}); | ||
| }).to['throw'](/Unknown line 3 "blah"/); |
There was a problem hiding this comment.
When I said positive case, I meant create a new test case that would be failing without the changes above. Basically I'd like to see a TDD artifact here so we both have a example of what this change fixes if someone looks back on this in the future. This also help protect yourself as it ensures that your use case doesn't regress in the future.
There was a problem hiding this comment.
Ok, I missed the point. I've added a test to check applyPatches() work with diffs without Index metadata, using oldFileName and newFileName fields, that's just my use case.
|
I think we're close to there, but something confused github and while I see your last commit in the commit list, I don't see the changes in the total change list for the PR. Haven't really seen that before and not sure what happened, but maybe if you squash the changes that will fix the issue. |
|
I've just review it at https://github.com/kpdecker/jsdiff/pull/88/files and changes have been applied, could you be able to take a look again? I'll be connected all afternoon if there's some problem. |
|
Is there any other thing that needs to be fixed so this get merged and published? If it's a problem with GitHub merge you could pull the changes from my repo, publish from there and close this pull-request by hand... I need to return my laptop by this weekend and would like to get this finished by then |
Split diffs based on file headers instead of 'Index:' metadata
|
Thank you, and sorry for my insistence. Could you be able to estimate when this will get published on npm? |
|
Released in 2.2.1 |
|
I've just checked it from npm and works correctly, thank you. |
No description provided.