-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Convert Project to Maven #2
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the Java tree is being moved, then the comments at the top of both the test Java files also need to be updated.
Also, are all those sections of pom.xml needed? There seems to be a lot of boilerplate copied from some other project. The smaller the footprint the less likely it is to break.
java/pom.xml
Outdated
|
||
<dependencies> | ||
<dependency> | ||
<groupId>junit</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a dependency on junit?
java/pom.xml
Outdated
<name>Google Diff Match and Patch</name> | ||
<url>https://github.com/google/diff-match-patch</url> | ||
<description>Diff Match Patch is a high-performance library in multiple languages that manipulates plain text.</description> | ||
<inceptionYear>XXXX</inceptionYear> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2006
java/pom.xml
Outdated
<description>Diff Match Patch is a high-performance library in multiple languages that manipulates plain text.</description> | ||
<inceptionYear>XXXX</inceptionYear> | ||
<organization> | ||
<url>http://www.google.com</url> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https here and three more below.
java/pom.xml
Outdated
<connection>scm:git:[email protected]:google/diff-match-patch</connection> | ||
<developerConnection>scm:git:[email protected]:google/diff-match-patch</developerConnection> | ||
<tag>google-analytics-java-1.1.2</tag> | ||
</scm> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation seems wrong here. Also, the rest of this file is a random mix of 2, 3, and 4 space indentations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will take care of it.
java/pom.xml
Outdated
<url>https://github.com/google/diff-match-patch</url> | ||
<connection>scm:git:[email protected]:google/diff-match-patch</connection> | ||
<developerConnection>scm:git:[email protected]:google/diff-match-patch</developerConnection> | ||
<tag>google-analytics-java-1.1.2</tag> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the analytics for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a copy and paste error. I will fix it.
@NeilFraser I have taken care of comments. About
You are right. I copied most of the template from my another project google-analytics-java. I copied with intention that sometime you would want to push this to library to maven central so that folks can easily consume it. However, Google may have different way of publishing to central (guava team might know about it). if that is the case, I can remove all that additional sections. |
@@ -18,9 +18,11 @@ | |||
|
|||
/** | |||
* Compile from diff-match-patch/java with: | |||
* javac -d classes src/name/fraser/neil/plaintext/diff_match_patch.java tests/name/fraser/neil/plaintext/diff_match_patch_test.java | |||
* javac -d target/classes src/main/java/name/fraser/neil/plaintext/diff_match_patch.java | |||
* javac -classpath target/classes -d target/test-classes src/test/java/name/fraser/neil/plaintext/diff_match_patch_test.java |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This second javac line gives me:
javac: directory not found: target/test-classes
[Oh, figured it out, one has to create three directories in advance.]
Also, corresponding changes are needed in Speedtest.java.
More generally, is there a reason why this directory tree is getting deeper and more complex? Is Maven unable to handle the previous structure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to configure maven to adopt previous structure. But how it is configured it is the standard. Also, since it is maven enabled, we would use maven to compile test. But since it is not Junit enabled, tests will not be picked to be run by Maven. I can convert the tests to Junit so you can just test with mvn test
@NeilFraser I have converted your tests to Junit based tests so that you can just execute them with command |
Converting the tests to JUnit (or any other test framework) will increase maintainability, not decrease. It is easier to spot issues given the fact that Most IDEs are optimized for it. Also the error message would be more readable; However I myself am not familiar with plugins, and don't use that many plugins. For one, I doubt whether Nexus Staging Maven Plugin are necessary in this pull request. |
java/pom.xml
Outdated
</build> | ||
</profile> | ||
</profiles> | ||
</project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing trailing new line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add new line
@FranklinYu Reason nexus related stuff is there with assumption that Neil would want to push this library to Maven Central so that is can be easily adopted by others. However since this google, they might have another internal approach they are following for other google libs like guava. If Neil wants to follow internal approach, I will remove all Nexus related stuff. |
Not familiar with the plugin, I assume that it automates pushing to Maven Central. However this repository does not seems to revolve that fast. I would suggest switching to Nexus only when manually pushing to Maven Central becomes a burden over @NeilFraser. Right now I think simply (manually) pushing the current state to Maven would be enough. |
Was there any development on this issue? I would be extremely advantageous to have the possibility to use this project via Maven Central. |
I would like to see an official release of this project on Maven too - I am currently importing an unofficial release (called "current") from a RedHat repository, but this updates dynamically: https://maven.repository.redhat.com/ga/diff_match_patch/diff_match_patch/current/ I would much rather import an official version with a proper version number, so I was delighted to see this pull request - however, it seems to have stalled. @brsanthu - I suggest you remove the Nexus stuff from |
@NeilFraser ready for another review. |
Whats stopping this from getting merged ? |
Please rebase your branch and also I would highly recommend upgrading the java version from 1.6 to atleast 1.8. |
About 1.8: If code is not using any 1.8 features, why make it that version as minimum? About rebasing: Unless Neil confirms that he will review and merge, there is no point in rebasing as he updates the code, it will get out of sync. |
Its not just about if the code does not use any 1.8 features. java 6 has had pretty much end of life for public updates a long time ago. Open source code does not have to keep using a version which will not have public updates anymore. |
Also, |
@mithuns I'd understand the merits of 1.8. But it is upto Neil to make that call. |
Ping? @NeilFraser |
And? Does the library now show up in maven central which is the objective if I understand right? |
@WolfgangFahl How is the group ID |
@FranklinYu |
Any update on this? I'd be happy to take a look if there is anything else that's needed. @NeilFraser @brsanthu |
I'd love to see this merged. It would improve our adoption of this project. |
Please let me know if you plan on publishing regular releases to Maven Central. If not, I will update https://bitbucket.org/cowwoc/google-diff-match-patch/ to do exactly that. I already published the google-code version a few years back but now there are requests to publish your newer releases. Obviously it's better for you to handle this in-house than have someone do this externally. Let me know... |
For those of you who are interested, I just pushed the latest version of this code to Maven Central under the name See https://bitbucket.org/cowwoc/google-diff-match-patch/ for the project website. NOTE: I am not developing this library. I am just releasing the latest code to Maven Central. |
Thanks @cowwoc for your efforts! It's great to have this library available as a regular gradle dependency - hopefully this PR moves forward but until then I've reviewed the cowwoc re-package and it appears to be a clean release of this code. Cheers |
@NeilFraser we're waiting on your response |
The author doesn’t seem interested any longer. I’m using a fork: https://github.com/java-diff-utils/java-diff-utils |
@NeilFraser - A bit of a tragedy is happening here! If this Maven PR could only merge, it would mean your labor of love would become instantly more appealing to a bigger swath of the Internet and your code would be more widely used and appreciated. But instead it's about to undergo a fork, splintering the user base and reducing the impact of your mainline efforts here. You've already done some work to review this PR-- can't we bring it back to life? |
What's the point of maintaining a java library if you're not going to make it accessible to the majority of java developers? |
Sad this is never taken care of ... |
@cowwoc Thanks for submitting the code to Maven. Would you (or anyone else) in this thread interested in the following modification: change the input type from If Let me know if somebody is interested in the patch / code. For above reason I cannot use java-diff-utils project, as in line 150 provided that |
@dmak did you mean
one-byte charsets like ISO-8859-1 are also problematic for this library until some specification on index/length units are made (I proposed one such backwards-compatible declaration in #83) for the output deltas. this is more or less to say, I agree with you that introducing a character stream reveals the complexity of representing human text, though technically it doesn't introduce complexity that wasn't already present-yet-hidden. |
You're right, I meant Anyway, just an idea that passing |
This PR converts the project to maven with support to be able to publish to Maven Central