(I don't think changing messages in existing IOs is a high priority, btw - just a nice-to-have, as long as we have guidance for future IOs)
On Fri, Jul 28, 2017 at 11:37 AM Eugene Kirpichov <kirpic...@google.com> wrote: > 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é <j...@nanthrax.net> > wrote: > >> Agree >> >> Regards >> JB >> >> On Jul 28, 2017, 20:22, at 20:22, Thomas Groh <tg...@google.com.INVALID> >> 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 < >> >kirpic...@google.com.invalid> 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"); >> >> ... >> >> } >> >> >> >
