Hi I'm a bit concerned because it has been discussed in a Jira and we discussed especially about this with Kenn and I. It dates from the incubation period.
I changes most of the IOs I did for style 2 whereas I started with style 1. I'm ready to change again on the IOs but we agreed that checkArgument and checkState are better to explain cause to the users more than checkNotNull. For me it's important to keep a coherence in the project. Either way is fine as far as it's consistent and it's not a single person decision. Regards JB On Jul 28, 2017, 20:18, at 20:18, 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"); > ... >}
