Extend FileSignature to be sensitive to file order and duplicates.

[fvgh]

FileSignature#fromSet corresponds to original behavior FileSignature#from.
FileSignature#fromList is sensitive to file order and duplicates.
Moved checks for 'null' files from caller into FileSignature

[fvgh]

@jbduncan Please have another look.
I implemented a modified version of your suggestions for the following reasons:

1. I don't use com.diffplug.common.collect.ImmutableList: According to the Project layout, the lib shall not have external dependencies.
2. Not allow null: Looking at the usage, I found that the FileSignature seem to prefer to adapt the input so that it can be used, rather than to throw exceptions in case of possible miss-configuration. Hence I think it would be better to remove null entries instead of throwing exceptions. Using this approach, I was able to remove corresponding code on the caller side.
3. Performance loss due to Iterable/List conversion: I agree to your point that I should in the beginning focus on clean code and to check performance issues at a later stage. Including your proposal, to iterate over the elements to check for null, I stuck to my initial approach to remove the ArraysList conversion from the FileSignature#toSortedSet. Have a look whether you can agree to my trade-off.

Sorry that I bothered you with my first PR. I had a better look now at all the callers this time. Let me know whether you think I still missed some of the intentions of the original code.

[jbduncan]

Sorry that I bothered you with my first PR.

Please, no worries! I realise that I overwhelmed you with feedback, so need to feel sorry. :)

2. Not allow null: Looking at the usage, I found that the FileSignature seem to prefer to adapt the input so that it can be used, rather than to throw exceptions in case of possible miss-configuration...

Yeah, we're pretty inconsistent with how we deal with nullable and non-null stuff in Spotless. :P

I'm happy that we skip nulls in this PR - I'll raise an issue later to make NPEs be thrown eagerly (my preferred method) more consistently.

All the other modifications you've made to my original proposal look very reasonable to me, except for,

1. using LinkedList instead of ArrayList in toNonNullList
2. removing the @SuppressWarnings("unused") annotations. I think they were there in the first place because those fields are reported by IDEs like IntelliJ to be "unused", whereas they really are being used, but just for serialization for Gradle.

Once those 2 minor things are fixed, I'm happy to merge this. :)

[fvgh]

OK, sorry, just saw why ArrayList makes more sense here.

[jbduncan]

Please explain why you prefer ArrayList over LinkedList in that situation. I wondered why ArrayList was used in the first place.
When we are using Iterable, we do not know the size of the input sequence. Hence I can not easily reserve an appropriate length for ArrayList.
In our scenarios we will not access the resulting list randomly.
So all in all, I see no advantage using ArrayList. Do you have any link, where I can read more about this issue, or can you provide me with some key-words I can use for further research?

As you discovered, it's because LinkedList isn't RandomAccess, whereas ArrayList is. And in this case, it matters whether it's random-access, because it gets returned by FileSignature::files. :)

If the sizing of the list really bothers you, consider changing asNonNullList to set a size for the ArrayList if the iterable is a Collection. E.g.:

/** Returns a shallow copy of input elements omitting null elements */
private static <T> List<T> asNonNullList(Iterable<T> input) {
	List<T> shallowCopy = 
		(input instanceof Collection)
			? new ArrayList<>(((Collection) input).size())
			: new ArrayList<>();
	 . . .
	return shallowCopy;
}

[fvgh]

Actually, I was not thinking about the RandomAccess, which we should not have.
I was more thinking about the previous/next pointer overhead.
But lets just reserve sufficient memory and I am happy :-)

[jbduncan]

Please put the @SuppressWarnings("unused") annotations you removed back in and squash your commits, and then we're ready to go.

[fvgh]

About the @SuppressWarnings("unused"):
Actually the problem was (also?) visible in Eclipse.
Problem is that in the original code the members were only set. Since I moved the calculation from the static method into the constructor, Eclipse was happy. I agree that it is a bug in Eclipse, but since the code change seems to make the current Eclipse version happy, do we want to keep the work-around?

[jbduncan]

Hmm, interesting. I'm not sure why moving the calculations from FileSignature.from to the constructor made Eclipse happy for you... :/

But yes, please keep the @SuppressWarning("unused") annotations in for now. I'll investigate it later on my version of IntelliJ IDEA (I'm not an Eclipse user), and if IntelliJ doesn't complain when they're removed, then I'll remove them myself. :)

[fvgh]

Maybe I should have mentioned that with the code change Eclipse now complains when I reinsert the @SuppressWarnings. Just installing IntelliJ IDEA, since I always wanted to try it.
Shall I anyway reintroduce the @SuppressWarnings, or can you gie me a sec to try it on IntelliJ IDEA myself?

[jbduncan]

I'm happy to give you the time you need to see how it looks with and without the annotations on IntelliJ.

If after seeing it you think I was mistaken in my assessment of needing the annotations, then I'd be very happy to be proven so!

Otherwise, if you agree with my assessment, add the annotations back in.

When that's done, squash your commits.

[fvgh]

Ok, back again.
The hint I get in IntelliJ with the old version is "Field is assigned, but never accessed".
But to my understanding of IntelliJ this is a hint and not a warning.
Anyhow, even the hint in IntelliJ is gone with the latest code change (since I access the filed in the for-loop). So I think the @SuppressWarnings was really used for Eclipse and not for IntelliJ.

[jbduncan]

I introduced the @SuppressedWarnings for IntelliJ originally, but regardless, I'm now convinced they're no longer needed! :)

[fvgh]

I am afraid I still have a few blind spots in my understanding of spotless and the whole set -up.
Can you confirm that Travis runs all tests?
I must admit that I skipped a few tests since I was not able to run them successfully on the master and neither on the branch. Still find it difficult to understand what Gradle does in terms of M2, and then on top spotless caching.

Update to original comment (misinterpreted the test, confused by test error):
The tests are working on my side with maven-local as long as only the current versions of the spotless libraries involved. The problems I see with mavenCentral are unrelated to the code changes and unrelated to this PR.

So the feature branch should be now in line with the previous agreements.

[nedtwigg]

Can you confirm that Travis runs all tests?

Travis runs .ci/ci.sh, which is gradlew build. So I can confirm that it runs all tests.

Still find it difficult to understand what Gradle does in terms of M2

If you're coming from a maven background, Gradle is a little different. It has its own, gradle-specific cache. Here is their blog post explaining why.

mavenCentral() = https://repo1.maven.org/maven2
mavenLocal() = ~/.m2

In maven, things from mavenCentral() automatically get cached into mavenLocal(), but in Gradle they get cached into the special gradle cache instead.

and then on top spotless caching.

Spotless caching (the whole TestProviders thing) is only for testing - nowhere else. In order for spotless up-to-date checks to work, we have to be very rigorous in checking equality between FormatterStep - which is the point of the SerializableEqualityTester.

The problem is that to resolve a JarState in a unit test, we have to make a fake Gradle build, add the dependency, and ask gradle to resolve it. It's very slow. Soooo, in order to make the unit tests faster, we added this little caching hack so that once a JarState has resolved once during testing, we won't have to create the fake Gradle build again.

gradlew clean clears these fake caches. If you suspect trouble, might wanna try a clean (gradle is fantastic about not needing arbitrary cleans, but our TestProvisioner hack is not nearly as robust).

I was not able to run them successfully on the master and neither on the branch.

Strange... What OS are you on? I just ran gradlew build on windows and linux for the current master, and it completed without a hitch (Travis is also happy).

Substance

This LGTM, except for two minor changes:

1. Keep the original methods, but mark as deprecated

/** Deprecated for {@link FileSignature::asList yada yada */
@Deprecated
public static FileSignature from(Iterable<File> files) throws IOException {
    return asList(files)
}

2. The from nomenclature is my fault, but I think it is now misleading.

// this looks easier to understand to me
Iterable<File> filesToSign = ...
FileSignature setSignature = FileSignature.signAsSet(filesToSign);
FileSignature listSignature = FileSignature.signAsList(filesToSign);
// than this
Iterable<File> filesToSign = ...
FileSignature setSignature = FileSignature.fromList(filesToSign);
FileSignature listSignature = FileSignature.fromSet(filesToSign);

[fvgh]

I implemented the requested changes.

@nedtwigg
Yes the clean is working, but I am reluctant to work on the whole project, since for me a fresh build takes a while. I introduced the mavenLocal to test my GrEclipse extension, but actually now I am using it more and more for quick testing within Eclipse. So if you don't consider it bad practice, I will still rely on Travis for a complete check.

About the checking of the entire project:
I was just not sure, since I saw in the beginning that '_ext' is excluded. But I think you do that for performance reasons, and consider the risk acceptable since it has only a very small interface with the rest of the project.

Thanks for the background information about Maven repository usage by Gradle and Spotless.

[jbduncan]

Looking back through the history of FileSignature, it was me who introduced the from static method (because I wanted to copy Google Guava's example for immutable data objects that don't directly contain the passed-in params, IIRC), so it might be my fault actually.

[jbduncan]

This LGTM now.

@nedtwigg, anything else you'd like to be done before I merge?

[nedtwigg]

Merge-ho!!!

On Sun, Mar 12, 2017 at 3:17 AM Jonathan Bluett-Duncan < ***@***.***> wrote: This LGTM now. @nedtwigg <https://github.com/nedtwigg>, anything else you'd like to be done before I merge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#85 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACyhwGmjC_MHNNhGXA0gQtb3yr51YpsUks5rk8ZDgaJpZM4MaMSc> .

-- Ned Twigg Lead Software Architect, DiffPlug LLC 540-336-8043 (cell) 888-513-6870 (fax) 340 S Lemon Ave #3433, Walnut, CA 91789

[jbduncan]

Done! Thank you very much for your work so far @fvgh. We'll be ready to accept your next PR whenever you're ready. :)

[fvgh]

Thanks again for your support and help. Hopefully I am getting into the work flow and more used to the code style so that there will be less effort on your side for the next PRs.