Okay, so then let's change guidance to: - Always use checkArgument to check stuff that the user supplied - Use style 1 for messages on the checkArgument ?
On Fri, Jul 28, 2017 at 11:31 AM Jean-Baptiste Onofré <[email protected]> wrote: > Agree > > Regards > JB > > On Jul 28, 2017, 20:22, at 20:22, 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 > > > >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"); > >> ... > >> } > >> >
