Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding functionality for retesting all when the classpath changes #13

Merged

Conversation

milicahtanovic
Copy link

continue;
}
String mapping = getJarToChecksumMapping(jarsList[i]);
writer.write(mapping + "\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use System.lineSeparator() instead of \n

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fixed.

InputStream is = Files.newInputStream(Paths.get(jar));
DigestInputStream dis = new DigestInputStream(is, md);
while (dis.read(bytes) != -1) {
;
Copy link
Collaborator

@august782 august782 Sep 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's with the empty loop? Is it purposely empty? It might look better without the brackets and just the semicolon.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not empty technically, it is filling the 'bytes' array.
I used 'while (dis.read(bytes) != -1);' but checkstyle complained, it requires {}.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is filling and then overwritting right? If the loop executes more than once you only get the last 8192 bytes and loose the prior ones, which may or may not be what you want.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. We are going to fix it and the whole algorithm in fact.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is fixed now.


byte[] mdbytes = md.digest();
for (int i = 0; i < mdbytes.length; i++) {
sb.append(Integer.toString((mdbytes[i] & 0xff) + 0x100 , 16).substring(1));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the goal of this math with hex values when making the String? Some comment could help.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I added the description now. It is computing the checksum of the jars.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changed to use commons-codec to get hex.

} catch (NoSuchAlgorithmException nsae) {
nsae.printStackTrace();
}
return jar + "," + sb.toString();
Copy link
Collaborator

@august782 august782 Sep 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're already using a StringBuilder, just use the StringBuilder to concatenate the Strings, instead of using the + (which will make a new StringBuilder anyway).

Also, in case of exceptions happening, the StringBuilder could be empty, right? Will this be accounted for later?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fixed it.
Yes, it could be empty and it would just write an empty line. Now I added a condition to skip if it could not generate the checksum.

if (sb.length() == 0) {
sb.append(paths[i]);
} else {
sb.append(":" + paths[i]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're already using a StringBuilder, it's best to just use the StringBuilder methods to concatenate Strings instead of using +.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To explain why these comments keep coming up, read this: http://www.pellegrino.link/2015/08/22/string-concatenation-with-java-8.html
It's a performance hit if you concatenate a lot of stuff... probably you don't but it's good to write good code.

private String cleanClassPath(String cp) {
String[] paths = cp.split(":");
StringBuilder sb = new StringBuilder();
for (int i = 0; i < paths.length; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ever possible for the paths to be in a different order? Is it necessary to sort first as to make the String always deterministic?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I will do set intersection instead?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are already putting them in sets just check for equality with equals()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

if (!sfPathString.equals(cleanOldSfPathFileName)) {
return false;
}
} catch (IOException ioe) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So upon IOException the return is true, meaning the classpaths would be considered the same. Is this what should happen?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to return true in the try block and false otherwise.

@@ -101,4 +114,58 @@ protected void setChangedAndNonaffected() throws MojoExecutionException {
nonAffectedTests = data == null ? new HashSet<String>() : data.getKey();
changedClasses = data == null ? new HashSet<String>() : data.getValue();
}

private boolean compareClassPaths(String sfPathString) throws MojoExecutionException {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change the name to instead of compareClassPaths to be something like checkIfSameClassPath, just to get the feel that the method returns a boolean.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it.

return true;
}

private boolean checkJarChecksums(String cleanSfClassPath) throws MojoExecutionException {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment about name as for compareClassPaths.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated that too.

return false;
}
}
} catch (IOException ioe) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment concerning exception and return as in compareClassPaths.

Copy link
Author

@milicahtanovic milicahtanovic Sep 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, added a flag to know if the exception happened and return false in that case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing with these exceptions is that they will happen on the first run always. We may want to silence them in that case?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check for file existence before the try:

if (!new File(oldChecksumPathFileName).exists()) {
     return false;
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the condition.

Copy link
Member

@alexgyori alexgyori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should decide if portability is a concern, and if it is, you should clean it up to make it more portable.
You could also set it up to run tests on AppVeyor and you'd find probably a whole bunch of these issues.

@@ -72,6 +77,22 @@ public static void writeClassPath(String sfPathString, String artifactsDir) {
}
}

public static void writeJarChecksums(String sfPathString, String artifactsDir) {
String outFilename = artifactsDir + File.separator + "jar-checksums";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's safer to not concatenate paths this way (e.g., artifactsDir may or may not end with a File.separator already). There is a method Paths.get(...).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using Paths.get(...) later in the code for this exact string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok... but is there a reason to not use it here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require to overload the method to which this string is passed and we don't want to do that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would still be good to use Paths.get(...), for same reasons as in other places with this pattern.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but as I said in this case it requires changing a method that is widely used within the whole project. This string gets passed to Paths.get(...) anyways.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand what widely used method is being changed here. Is it not enough to just change artifactDir + File.separator + "jar-checksum" to Paths.get(artifactDir, "jar-checksum").toString()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

public static void writeJarChecksums(String sfPathString, String artifactsDir) {
String outFilename = artifactsDir + File.separator + "jar-checksums";
try (BufferedWriter writer = getWriter(outFilename)) {
String[] jarsList = sfPathString.split(":");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In unix systems the path separator is : but in windows it is ; (semicolon). To be portable use File.pathSeparator which gives you the right thing. As a rule of thumb, whenever you find yourself using some string literal ask if that's portable. If you set it yourself, it's fine; if it comes from the platform, check.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not done here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, didn't see that. Changed it now.

String outFilename = artifactsDir + File.separator + "jar-checksums";
try (BufferedWriter writer = getWriter(outFilename)) {
String[] jarsList = sfPathString.split(":");
for (int i = 0; i < jarsList.length; i++) {
Copy link
Member

@alexgyori alexgyori Sep 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use an enhanced for loop here to be more java-ic (is that the equivalent of pythonic?).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but it will eventually be broken down to this so it's not that important.

InputStream is = Files.newInputStream(Paths.get(jar));
DigestInputStream dis = new DigestInputStream(is, md);
while (dis.read(bytes) != -1) {
;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is filling and then overwritting right? If the loop executes more than once you only get the last 8192 bytes and loose the prior ones, which may or may not be what you want.

String[] elems = line.split(",");
checksumMap.put(elems[0], elems[1]);
}
String[] jars = cleanSfClassPath.split(":");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use pathSeparator if needed...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, changed it.

private String cleanClassPath(String cp) {
String[] paths = cp.split(":");
StringBuilder sb = new StringBuilder();
for (int i = 0; i < paths.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enhanced for loop to be more java-ic.

String[] paths = cp.split(":");
StringBuilder sb = new StringBuilder();
for (int i = 0; i < paths.length; i++) {
if (paths[i].contains("/target/classes") || paths[i].contains("/target/test-classes")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is definitely not portable; in windows you'd have \.

StringBuilder sb = new StringBuilder();
for (int i = 0; i < paths.length; i++) {
if (paths[i].contains("/target/classes") || paths[i].contains("/target/test-classes")
|| paths[i].contains("-SNAPSHOT.jar")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

endsWith instead of contains maybe?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will consider that.

if (sb.length() == 0) {
sb.append(paths[i]);
} else {
sb.append(":" + paths[i]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To explain why these comments keep coming up, read this: http://www.pellegrino.link/2015/08/22/string-concatenation-with-java-8.html
It's a performance hit if you concatenate a lot of stuff... probably you don't but it's good to write good code.

if (sb.length() == 0) {
sb.append(paths[i]);
} else {
sb.append(":" + paths[i]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to use pathSeparator here to be portable? i.e., it's : on unix and ; on windows?

@@ -72,6 +77,22 @@ public static void writeClassPath(String sfPathString, String artifactsDir) {
}
}

public static void writeJarChecksums(String sfPathString, String artifactsDir) {
String outFilename = artifactsDir + File.separator + "jar-checksums";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok... but is there a reason to not use it here?

public static void writeJarChecksums(String sfPathString, String artifactsDir) {
String outFilename = artifactsDir + File.separator + "jar-checksums";
try (BufferedWriter writer = getWriter(outFilename)) {
String[] jarsList = sfPathString.split(":");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not done here.

if (mapping.split(",").length < 2) {
continue;
}
writer.write(mapping + System.lineSeparator());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have two calls to write rather than concatenate (which creates a new StringBuilder...)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -101,4 +114,69 @@ protected void setChangedAndNonaffected() throws MojoExecutionException {
nonAffectedTests = data == null ? new HashSet<String>() : data.getKey();
changedClasses = data == null ? new HashSet<String>() : data.getValue();
}

private boolean checkIfSameClassPath(String sfPathString) throws MojoExecutionException {
String oldSfPathFileName = getArtifactsDir() + "sf-classpath";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly here you are assuming that the path returned by the getArtifactsDir ends with / or . If it doesn't this path is incorrect. Use Paths.get(...) maybe.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added File.separator.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what Alex is saying is that you would never be sure that getArtifactsDir() actually returns a String that ends with File.separator; later somebody could change it, or someone reading this code would never be sure that it ends with File.separator (and would need to jump 2 or 3 files just to double-check, which I did...). Just to be safe, it would be good to explicitly use something like Paths.get(...). As of now, it does indeed work though, since getArtifactsDir() does end with File.separator.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in the newer version.

String[] paths = cp.split(File.pathSeparator);
StringBuilder sb = new StringBuilder();
for (int i = 0; i < paths.length; i++) {
if (paths[i].contains("/target/classes") || paths[i].contains("/target/test-classes")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These paths don't quite work on Windows.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I used the File.separator-s here too.

@owolabileg owolabileg requested a review from denizarsan November 6, 2017 15:07
@owolabileg
Copy link
Collaborator

@august782 @denizarsan @alexgyori I'd like to merge this ASAP. Please take one final look.

@@ -117,12 +117,14 @@ protected void setChangedAndNonaffected() throws MojoExecutionException {

private boolean checkIfSameClassPath(String sfPathString) throws MojoExecutionException {
String oldSfPathFileName = getArtifactsDir() + "sf-classpath";
Set<String> sfClassPathSet = new HashSet<>(Arrays.asList(sfPathString.split(":")));
Set<String> sfClassPathSet = new HashSet<>(Arrays.asList(sfPathString.split(File.pathSeparator)));
if (!new File(oldSfPathFileName).exists()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this conditional check one line earlier, before setting sfClassPathSet, as if it does not exist, making that Set is not necessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Set<String> sfClassPathSet = new HashSet<>(Arrays.asList(sfPathString.split(File.pathSeparator)));
if (!new File(oldSfPathFileName).exists()) {
return false;
}
try {
String cleanOldSfPathFileName = cleanClassPath(Files.readAllLines(Paths.get(oldSfPathFileName)).get(0));
Set<String> oldSfClassPathSet = new HashSet<>(Arrays.asList(cleanOldSfPathFileName.split(":")));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as before about : versus File.pathSeparator.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed.

@@ -135,13 +137,16 @@ private boolean checkIfSameJarChecksums(String cleanSfClassPath) throws MojoExec
String oldChecksumPathFileName = getArtifactsDir() + "jar-checksums";
Map<String, String> checksumMap = new HashMap<>();
boolean noException = true;
if (!new File(oldChecksumPathFileName).exists()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as before, moving conditional closer to where it should be checked to exit earlier.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved.

@@ -101,4 +114,69 @@ protected void setChangedAndNonaffected() throws MojoExecutionException {
nonAffectedTests = data == null ? new HashSet<String>() : data.getKey();
changedClasses = data == null ? new HashSet<String>() : data.getValue();
}

private boolean checkIfSameClassPath(String sfPathString) throws MojoExecutionException {
String oldSfPathFileName = Paths.get(getArtifactsDir() + File.separator + "sf-classpath");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are using Paths.get(...), you then don't need to append File.separator, you can just use Paths.get(getArtifactsDir(), "sf-classpath"), with the parts as arguments.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't gonna include this in the commit. I am not sure I want to use Path.get(...). I just need a string here. I will remove Path.get(...).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will actually need to put .toString() at the end to make it work for the String. I would still recommend it since Paths.get(...) has the logic for determining whether or not to use the File.separator when connecting the parts, leading to a cleaner String.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

String cleanOldSfPathFileName = cleanClassPath(Files.readAllLines(Paths.get(oldSfPathFileName)).get(0));
Set<String> sfClassPathSet = new HashSet<>(Arrays.asList(sfPathString.split(File.pathSeparator)));
Set<String> oldSfClassPathSet = new HashSet<>(Arrays.asList(cleanOldSfPathFileName.split(File.pathSeparator)));
if (oldSfClassPathSet.equals(oldSfClassPathSet)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is checking if oldSfClassPathSet is equal to itself. I believe you need it to be some other variable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I fix that on the other branch. Good catch.

continue;
}
String mapping = getJarToChecksumMapping(jarsList[i]);
if (mapping.split(",").length < 2) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does "2" come from?

Copy link
Author

@milicahtanovic milicahtanovic Nov 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how I save them when I write to the file -> jar,checksum.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining this? It will make things easier for others or for you in the future.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

public static String getJarToChecksumMapping(String jar) {
StringBuilder sb = new StringBuilder();
sb.append(jar);
sb.append(",");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can chain these two appends for readability.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By chaining, do you mean something like sb.append(jar + ",")? I would actually not recommend that, since underneath that would create a new StringBuilder just to concatenate the two Strings, and since we already have a StringBuilder going it is a bit unoptimal.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I mean sb.append(jar).append(“,”).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -101,4 +114,69 @@ protected void setChangedAndNonaffected() throws MojoExecutionException {
nonAffectedTests = data == null ? new HashSet<String>() : data.getKey();
changedClasses = data == null ? new HashSet<String>() : data.getValue();
}

private boolean checkIfSameClassPath(String sfPathString) throws MojoExecutionException {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about naming this "isSameClassPath" instead?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

return false;
}

private boolean checkIfSameJarChecksums(String cleanSfClassPath) throws MojoExecutionException {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before: "hasSameJarChecksum"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

return noException;
}

private String cleanClassPath(String cp) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same: "getCleanClassPath"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

for (int i = 0; i < paths.length; i++) {
if (paths[i].contains(File.separator + "target" + File.separator + "classes")
|| paths[i].contains(File.separator + "target" + File.separator + "test-classes")
|| paths[i].contains("-SNAPSHOT.jar")) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contains -> endsWith

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I considered that but I don't think it makes much difference.

sb.append(paths[i]);
} else {
sb.append(File.pathSeparator);
sb.append(paths[i]);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can chain these two appends for readability.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about comment about chaining from before.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reply: I mean sb.append(foo).append(bar). Sorry if it wasn’t clear.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

String[] paths = cp.split(File.pathSeparator);
StringBuilder sb = new StringBuilder();
for (int i = 0; i < paths.length; i++) {
if (paths[i].contains(File.separator + "target" + File.separator + "classes")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small optimization, it may be good to create the one String that represents File.separator + "target" + ... outside the loop, so there is no String concatenation on every iteration of the loop.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

private boolean checkIfSameJarChecksums(String cleanSfClassPath) throws MojoExecutionException {
String oldChecksumPathFileName = getArtifactsDir() + File.separator + "jar-checksums";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment concerning using Paths.get(...).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I taught that the problem was the path separator?
What is the actual suggestion?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggestion is to do "String oldChecksumPathFileName = Paths.get(getArtifactsDir(), "jar-checksums").toString();"
The reason is because it is unclear (without spending some time looking) whether or not getArtifactsDir() has the File.separator at the end or not. If it does, it leads to a kind of ugly String with the double /. While this might not break functionality, it is usually better to use some utility methods that provide the benefit of the checking.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

@alexgyori alexgyori Nov 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if you have: "/home/milicah/" + File.separator + "something"?
You will have two /, i.e., //, in the middle of the string. Which may or may not break things depending on what you do. The first string may or may not have / at the end, you either put an assertion that ensures the first string doesn't end with /, or use Paths.get everywhere which has the logic to handle all these edge cases which we come up with because we actually ran into them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually had that at some point. It didn't brake anything, but yeah it could.
I changed this to use Paths.get(...).

@milicahtanovic
Copy link
Author

@alexgyori @august782 @denizarsan @owolabileg All the comments have been addressed, I think this branch is ready to merge.

@denizarsan
Copy link

@milicahtanovic You have some conflicts. Please resolve them before merging.

@august782
Copy link
Collaborator

I don't think all comments have been addressed yet. Please at least make sure you have responded to each comment and answer any questions or acknowledge the comment.

@milicahtanovic
Copy link
Author

Oh, I didn't see them. For some reason they didn't show up on the discussion board. I will go over this, sorry.

@august782
Copy link
Collaborator

Please confirm on some small examples that the functionality is still working, that our suggestions did not break anything; try modifying the classpath and JARs on the classpath to test. After that, I believe it should be ready to merge.

@milicahtanovic
Copy link
Author

I tested with changing the dependency versions and it behaves as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants