modernizer is at odds with guava recommendations

[hgschmie]

According to https://code.google.com/p/guava-libraries/wiki/PreconditionsExplained, the guideline if using Guava in JDK7+ is

--- snip ---
We preferred rolling our own preconditions checks over e.g. the comparable utilities from Apache Commons for a few reasons. Piotr Jagielski discusses why he prefers our utilities, but briefly:

After static imports, the Guava methods are clear and unambiguous. checkNotNull makes it clear what is being done, and what exception will be thrown.
checkNotNull returns its argument after validation, allowing simple one-liners in constructors: this.field = checkNotNull(field).
Simple, varargs "printf-style" exception messages. (This advantage is also why we recommend continuing to use checkNotNull over Objects.requireNonNull introduced in JDK 7.)
--- snip ---

which clashes with https://github.com/andrewgaul/modernizer-maven-plugin/blob/master/src/main/resources/modernizer.xml#L597-L607

[electrum]

As a bit of data, I checked the Presto code base, where we previously converted from checkNotNull to requireNonNull. We have over 4600+ instances of requireNonNull and less than 25 which have a non-constant message, of which most should actually be checkArgument, since they're doing something like this:

Type type = requireNonNull(types.get(symbol), "No type for symbol " + symbol);

Also note that you can pass a Supplier for the message:

Type type = requireNonNull(types.get(symbol), () -> "No type for symbol " + symbol);

Thus, my opinion is that the consistency of using the JDK method everywhere outweighs the advantages of the Guava format string (which does have the nice feature of being safe even if you mess up the format args).

[gaul]

@kevinb9n any thoughts on this? I would like to close this issue if not.

[kevinb9n]

Sorry I missed that question.

So yeah, I think checkNotNull has some advantages over requireNonNull, especially if the codebase also uses checkArgument/checkState. For that matter, I also believe in the value of Guava's Preconditions.checkNotNull / Verify.verifyNotNull distinction, so that a stack trace always portrays very clearly whether it is the caller's fault or not. But I doubt many projects care that much about exception hygiene, so <shrug>.

I know there are a lot of projects that were only using the parts of Guava that now have JDK analogues, and it's easy to understand why they would want to shed all their remaining Guava usages and jettison the dependency.

[hgschmie]

I wonder if adding Supplier variants to the various Verify/Precondition methods would be possible. E.g.

Preconditions.checkNotNull(T obj, Supplier<String> messageSupplier), similar to what exists for requireNonNull. Having those for checkNotNull/checkArgument/checkState and Verify.verify/verifyNotNull might be valuable.

[kevinb9n]

@hgschmie see google/guava#5927