Makes sense.
On 2020/12/09 22:44:35, Stamatis Zampetakis <zabe...@gmail.com> wrote: > Sounds reasonable and shares some goals with JEP358 [1] so why not. > > [1] https://openjdk.java.net/jeps/358 > > On Wed, Dec 9, 2020 at 8:26 AM Vladimir Sitnikov < > sitnikov.vladi...@gmail.com> wrote: > > > Hi, > > > > I suggest we use requireNonNull(var, "var") pattern rather > > than requireNonNull(var). > > > > Then error message would include which variable/field turned out to be > > null. > > It would make a big difference in case a constructor verifies multiple > > parameters like in Correlate: > > > > protected Correlate( > > RelOptCluster cluster, > > RelTraitSet traitSet, > > RelNode left, > > RelNode right, > > CorrelationId correlationId, > > ImmutableBitSet requiredColumns, > > JoinRelType joinType) { > > super(cluster, traitSet, left, right); > > assert !joinType.generatesNullsOnLeft() : "Correlate has invalid join > > type " + joinType; > > this.joinType = requireNonNull(joinType); > > this.correlationId = requireNonNull(correlationId); > > this.requiredColumns = requireNonNull(requiredColumns); > > > > If a user makes a mistake and passes null to joinType or correlationId, > > then Calcite would throw NPE, however, it would be hard to tell which > > parameter was null. > > Line numbers are known to drift over time. > > > > I suggest we use the following pattern consistently: > > > > this.joinType = requireNonNull(joinType, "joinType"); > > this.correlationId = requireNonNull(correlationId, "correlationId"); > > this.requiredColumns = requireNonNull(requiredColumns, > > "requiredColumns"); > > > > The PR is https://github.com/apache/calcite/pull/2293 > > > > The suggested change automatically aligns single-word message with the > > first argument to requireNonNull. > > In other words, "./gradlew style" command would insert/fix the messages > > automatically. > > I assume modern IDEs enable developers to configure var => > > requireNonNull(var, "var") expansion (I use it in IDEA). > > > > Currently we have > > 545 usages of requireNonNull(var) <-- this is to be updated to (var, "var") > > 359 usages of requireNonNull(var, "var") > > 37 usages of requireNonNull(var, () -> ..) > > 33 usages of requireNonNull(var, "custom message") > > > > If no-one objects within three days, I'll assume lazy consensus and commit > > it. > > > > Vladimir > > >
