On Fri, Jul 28, 2017 at 11:21 AM, Thomas Groh <[email protected]> wrote:
> I'm in favor of the wording in the style of the first: it's an immediately > actionable message that will have an associated stack trace, but will > provide the parameter in plaintext so the caller doesn't have to dig > through the invoked code, they can just look at the documentation. > > I've recently been convinced that all input validation should go through > `checkArgument` (including for nulls) rather than 'checkNotNull', due to > the type of exception thrown, so I'd usually prefer using that as the > `Preconditions` method. Beyond that, +1 > +1. For me, main requirement is that it should be aimed at end user and should be actionable without having to look at the code. It is always a delight to receive message useful error as early as possible. This also applies when checks are made in validate(). > > On Fri, Jul 28, 2017 at 11:17 AM, Eugene Kirpichov < > [email protected]> wrote: > > > Hey all, > > > > I think this has been discussed before on a JIRA issue but I can't find > it, > > so raising again on the mailing list. > > > > Various IO (and non-IO) transforms validate their builder parameters > using > > Preconditions.checkArgument/checkNotNull, and use different styles for > > error messages. There are 2 major styles: > > > > 1) style such as: > > checkNotNull(username, "username"), or checkArgument(username != null, > > "username can not be null") or checkArgument(username != null, > > "username must be set"); > > checkArgument(batchSize > 0, "batchSize must be non-negative, but was: > %s", > > batchSize) > > > > 2) style such as: > > checkArgument( > > username != null, > > "ConnectionConfiguration.create().withBasicCredentials( > > username, > > password) " > > + "called with null username"); > > checkArgument( > > !username.isEmpty(), > > "ConnectionConfiguration.create().withBasicCredentials( > > username, > > password) " > > + "called with empty username"); > > > > Style 2 is recommended by the PTransform Style Guide > > https://beam.apache.org/contribute/ptransform-style-guide/#transform- > > configuration-errors > > > > However: > > 1) The usage of these two styles is not consistent - both are used in > about > > the same amounts in Beam IOs. > > 2) Style 2 seems unnecessarily verbose to me. The exception thrown from a > > checkArgument or checkNotNull already includes the method being called > into > > the stack trace, so I don't think the message needs to include the > method. > > 3) Beam is not the first Java project to have validation of configuration > > parameters of something or another, and I don't think I've seen something > > as verbose as style 2 used anywhere else in my experience of writing > Java. > > > > What do people think about changing the guidance in favor of style 1? > > > > Specifically change the following example: > > > > public Twiddle withMoo(int moo) { > > checkArgument(moo >= 0 && moo < 100, > > "Thumbs.Twiddle.withMoo() called with an invalid moo of %s. " > > + "Valid values are 0 (inclusive) to 100 (exclusive)", > > moo); > > return toBuilder().setMoo(moo).build();} > > > > into the following: > > > > public Twiddle withMoo(int moo) { > > checkArgument(moo >= 0 && moo < 100, > > "Valid values for moo are 0 (inclusive) to 100 (exclusive), " > > + "but was: %s", > > moo); > > return toBuilder().setMoo(moo).build();} > > > > > > And in simpler cases such as non-null checks: > > public Twiddle withUsername(String username) { > > checkNotNull(username, "username"); > > checkArgument(!username.isEmpty(), "username can not be empty"); > > ... > > } > > >
