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"); >> ... >> } >>
