HTML Reporter: New Diff using Google's Diff-Patch-Match Library #772
Conversation
|
This project uses a Apache 2.0 License. Regarding that, I believe the best approach would be to have an exposed copy of that library including its license. |
|
@leobalter The library is big and contains a lot of things not required. Doesn't this come under the modification of code? |
|
Section 4a of the license:
|
| * | ||
| * More Info: | ||
| * http://ejohn.org/projects/javascript-diff-algorithm/ | ||
| * https://code.google.com/p/google-diff-match-patch/ |
There was a problem hiding this comment.
It might be possible to paste the license inline here, I'm not sure about it. Would you confirm this, @scottgonzalez?
|
Where was this source obtained from? It doesn't look like https://code.google.com/p/google-diff-match-patch/source/browse/trunk/javascript/diff_match_patch_uncompressed.js. |
|
@scottgonzalez Its from that file. Converted all var to camelcase, grouped the variables, removed unnecessary functions. |
😖 Sounds pretty terrible for future upgrades. Besides the removal of unused functions, why would you even do that? |
|
I guess the idea is that we'll never upgrade because the project is no longer active? |
|
@scottgonzalez Which project? |
|
The one you just copy and pasted... |
|
@scottgonzalez u mean the diff-patch-match? |
Yes. I assume the thought is that the project is dead and we'll never need to update, so changing source is ok. |
|
@scottgonzalez So should we include the license as @leobalter suggested? |
|
It's a fairly lengthy license to include in source. I'd prefer if it were in its own file like we do in every other project. |
|
@scottgonzalez SO create a new file with a licence file in it? Should i place it in the external/jsdiff folder?? |
|
Yes, though we should probably rename the file and directory since it no longer contains jsdiff. |
|
@scottgonzalez yes ... thanks :) |
|
@scottgonzalez Should we name it diff-match-patch?? |
|
Well, with how heavily modified it is, I'd personally move it back to |
|
@scottgonzalez ok ... i will wait for @jzaefferer to reply before making any changes |
|
The last commit is from 2012, and while issues are still getting reported, they're not getting addressed. With that in mind I agree with Scott that the project is effectively dead. Considering the modifications I also agree that it makes sense to deal with this as source code, putting it into I'm not exactly sure how to deal with the license in this case, though I noticed that the Apache 2.0 license appendix provides a brief version to put in source code, which seems pretty reasonable:
On the implementation/testing side, we should do more testing, especially with this in mind (compare html/xml strings): https://code.google.com/p/google-diff-match-patch/wiki/Plaintext |
|
Right, that's basically what's in the original source. My first suggestion was going to be to keep that intact, but the code looked so different that I instead asked where the code came from. The Apache 2.0 license has the following clause:
I'm not sure how literal this needs to be followed, i.e., must we copy the existing license block verbatim or can we just reproduce the same details in our own format or by following the standard template? I highly doubt this has been tried in court, so it's going to be up for debate. @StevenAyr what do you suggest? The other clause we need to address is:
|
|
I don't think anyone would ever sue because you put the license in the License file and didn't leave it inline, but if it's just as easy to leave it inline that may ultimately be easier and cleaner than adding something new to the License file anyways. Ultimately, the biggest consideration is probably just keeping things uniform across all projects and files, so go with the method that's otherwise used. As for the modification notice, this too can live either in the license file or inline, depending on what you guys think is the best practice. Again though, it may be cleanest to put something inline along the lines of: |
|
Adding a notification to the source file and the proper license (or at least what the appendix suggests) to LICENSE.txt sounds good to me. How is this for the notification?
|
|
I think it's probably best to go ahead and put the full appendix in there, just to be technically compliant. |
|
I've updated my comment above, to include the full appendix. How's that? |
|
@leobalter @jzaefferer Please review |
| * limitations under the License. | ||
| * | ||
| * More Info: | ||
| * https://code.google.com/p/google-diff-match-patch/ |
There was a problem hiding this comment.
http://google-opensource.blogspot.com/2015/03/farewell-to-google-code.html
This came up today, what should we do with the Google Code links when it expires?
There was a problem hiding this comment.
Maybe file an issue on the project to ask about migration, and contact the author directly as well? As long as its unclear if they'll host a proper archive, getting him to migrate to another platform would be best.
There was a problem hiding this comment.
People on twitter are asking them to keep an archive, but so far on their agenda:
January 25, 2016 - The project hosting service is closed. You will be able to download a tarball of project source, issues, and wikis. These tarballs will be available throughout the rest of 2016.
So far, I'll give my ok to these lines here and we can update it later if we have any other resolution from Google Code.
There was a problem hiding this comment.
Apache License, Version 2.0 doesn't require us to link to the original source or even provide the original Source form with the distribution. I'd say we don't need to spend any more time worrying about this. If you want to ensure the original Source form is always available, just provide a copy in the external directory.
| the terms above. | ||
| All files located in the node_modules are externally maintained | ||
| libraries used by this software which have their own licenses; we | ||
| recommend you read them, as their terms may differ from the terms above. |
There was a problem hiding this comment.
@jzaefferer Is this correct??
There was a problem hiding this comment.
Missing "directory", should be "All files located in the node_modules directory are [...]"
|
@jzaefferer Please Review |
| * | ||
| * Usage: QUnit.diff(expected, actual) | ||
| * | ||
| * QUnit.diff( "the quick brown fox jumped over", "the quick fox jumps over" ) === "the quick <del>brown </del> fox <del>jumped </del><ins>jumps </ins> over" |
There was a problem hiding this comment.
Is this example still correct? The new diff might output something else here.
|
Pretty close! |
|
@jzaefferer Please Review. |
|
Awesome! LGTM |
|
Fixed the License file statement, and the example in the comment of src/diff.js |
Did you do this in a new commit without pushing it? |
|
no, i did this in the last commit that i pushed, gauravmittal1995@39b40ff (The fifth commit in this PR). Just mentioning the changes done in the commit |
|
@gauravmittal1995 mostly off-topic: When are you going to submit your proposal through the GSoc site? |
|
@jzaefferer I am in the middle of my Mid Semester Exams which will last till friday. I have made a profile at the Melange Site. I mostly will submit my proposal by sunday. Any thoughts?? |
|
That's fine, just wouldn't want you to miss the deadline (27th). |
|
@jzaefferer Yes, Thanks :) ... My exams just got over. Will submit it by tomorrow (Saturday). |
|
Merging it, the only other point is, a lot of other diff PRs might be affected. Now that we have a more consistent diff library, it's also good to create tests for it, I'll save this in a new issue. |
Uses google's Diff Patch Match Library to replace the current diff algorithms.
Solves: #348
Discussed (and screenshots) in #764
@leobalter @jzaefferer Please Review