Re: Handling nulls for cumulative task arg values.

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Handling nulls for cumulative task arg values.

Szczepan Faber-2
On Mon, Mar 3, 2014 at 9:21 AM, Luke Daley <[hidden email]> wrote:
Hi,

|task blowUp(type: JavaExec) {
    main = 'MainClass'
    classpath = sourceSets.main.runtimeClasspath
    args null
}|

 This produces a less-than-user-friendly NPE:

|Caused by: java.lang.NullPointerException
    at org.gradle.util.GUtil.addToCollection(GUtil.java:135)
    at org.gradle.process.internal.JavaExecHandleBuilder.args(JavaExecHandleBuilder.java:179)
    at org.gradle.api.tasks.JavaExec.args(JavaExec.java:261)
    at build_7p31187sv6j4incv2nd9fufiog$_run_closure1.doCall(/home/kamils/dev/sandbox/gradle/null_arg/build.gradle:6)|


We have a PR that proposes just silently ignoring nulls here and in other similar cases: https://github.com/gradle/gradle/pull/251

I'm not sure what we want to do here. Silently ignoring nulls doesn't sit well with me. I'd prefer we validate the args early and not allow nulls. We don't seem to do this anywhere though that I could find.

So:

1. Is this current situation good enough?

-1
 
2. Should we just silently drop nulls?

-1
 
3. Should we establish patterns/methods for validation and better error messages in this case?

+1 IMHO, Ignoring nulls unavoidably leads inconsistent apis because we won't be able to ignore them everywhere.

Cheers!
--
Szczepan Faber
Principal engineer@gradle; Founder@mockito
Reply | Threaded
Open this post in threaded view
|

Re: Handling nulls for cumulative task arg values.

Rob Platt
 
3. Should we establish patterns/methods for validation and better error messages in this case?

+1 from me too.

One option would be to use bean validation 1.1 on public API methods. If gradle can inject the validator appropriately, then this would be very powerful, as gradle could also catch the validation exceptions for better presentation, translations can be provided, etc.

Or a more "old-skool" approach can be taken. Check for null at the entry point of the public API, and throw an exception with a friendier message. In this case it'd be org.gradle.api.tasks.JavaExec.args. If args is null:

throw new NullPointerException("JavaExec cannot be given null args")

or

throw new IllegalArgumentException("JavaExec cannot be given null args")

I tend to prefer the IllegalArgumentException. In my experience developers assume that a NullPointerException is only thrown by the JVM and is a sign that the called code is "bad", whereas an IllegalArgumentException makes it clear that the API didn't like what the client gave it. Also, you can be consistent and use IllegalArgumentException for other validation checks (empty strings, negative values, etc.).

But bean validation 1.1 would be awesome.

Regards
Rob
Reply | Threaded
Open this post in threaded view
|

Re: Handling nulls for cumulative task arg values.

Adam Murdoch
In reply to this post by Szczepan Faber-2

On 3 Mar 2014, at 7:21 pm, Luke Daley <[hidden email]> wrote:

Hi,

|task blowUp(type: JavaExec) {
   main = 'MainClass'
   classpath = sourceSets.main.runtimeClasspath
   args null
}|

This produces a less-than-user-friendly NPE:

|Caused by: java.lang.NullPointerException
   at org.gradle.util.GUtil.addToCollection(GUtil.java:135)
   at org.gradle.process.internal.JavaExecHandleBuilder.args(JavaExecHandleBuilder.java:179)
   at org.gradle.api.tasks.JavaExec.args(JavaExec.java:261)
   at build_7p31187sv6j4incv2nd9fufiog$_run_closure1.doCall(/home/kamils/dev/sandbox/gradle/null_arg/build.gradle:6)|


We have a PR that proposes just silently ignoring nulls here and in other similar cases: https://github.com/gradle/gradle/pull/251

I'm not sure what we want to do here. Silently ignoring nulls doesn't sit well with me. I'd prefer we validate the args early and not allow nulls. We don't seem to do this anywhere though that I could find.

So:

1. Is this current situation good enough?

No

2. Should we just silently drop nulls?

No

3. Should we establish patterns/methods for validation and better error messages in this case?

Yes.

I’d change the decoration and/or DSL layer to deal with this as a cross-cutting thing. We also want to something very similar for dealing with mutability during the object’s lifecycle.


--
Adam Murdoch
Gradle Co-founder
http://www.gradle.org
VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com



Reply | Threaded
Open this post in threaded view
|

Re: Handling nulls for cumulative task arg values.

Luke Daley-2


> Adam Murdoch <mailto:[hidden email]>
> 4 March 2014 8:19 am
>
> On 3 Mar 2014, at 7:21 pm, Luke Daley <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>> Hi,
>>
>> |task blowUp(type: JavaExec) {
>>    main = 'MainClass'
>>    classpath = sourceSets.main.runtimeClasspath
>>    args null
>> }|
>>
>> This produces a less-than-user-friendly NPE:
>>
>> |Caused by: java.lang.NullPointerException
>>    at org.gradle.util.GUtil.addToCollection(GUtil.java:135)
>>    at
>> org.gradle.process.internal.JavaExecHandleBuilder.args(JavaExecHandleBuilder.java:179)
>>    at org.gradle.api.tasks.JavaExec.args(JavaExec.java:261)
>>    at
>> build_7p31187sv6j4incv2nd9fufiog$_run_closure1.doCall(/home/kamils/dev/sandbox/gradle/null_arg/build.gradle:6)|
>>
>>
>> We have a PR that proposes just silently ignoring nulls here and in
>> other similar cases: https://github.com/gradle/gradle/pull/251
>>
>> I'm not sure what we want to do here. Silently ignoring nulls doesn't
>> sit well with me. I'd prefer we validate the args early and not allow
>> nulls. We don't seem to do this anywhere though that I could find.
>>
>> So:
>>
>> 1. Is this current situation good enough?
>
> No
>
>> 2. Should we just silently drop nulls?
>
> No
>
>> 3. Should we establish patterns/methods for validation and better
>> error messages in this case?
>
> Yes.
>
> I’d change the decoration and/or DSL layer to deal with this as a
> cross-cutting thing. We also want to something very similar for
> dealing with mutability during the object’s lifecycle.
Right, unsure what to do with the PR then. Probably another case of
close saying we want to fix this more thoroughly in the future. There's
a low chance the OP will want to help on this, but I'll propose.

>
> --
> Adam Murdoch
> Gradle Co-founder
> http://www.gradle.org
> VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting
> http://www.gradleware.com
>
>
>
> Luke Daley <mailto:[hidden email]>
> 3 March 2014 6:21 pm
> Hi,
>
> |task blowUp(type: JavaExec) {
>     main = 'MainClass'
>     classpath = sourceSets.main.runtimeClasspath
>     args null
> }|
>
>  This produces a less-than-user-friendly NPE:
>
> |Caused by: java.lang.NullPointerException
>     at org.gradle.util.GUtil.addToCollection(GUtil.java:135)
>     at
> org.gradle.process.internal.JavaExecHandleBuilder.args(JavaExecHandleBuilder.java:179)
>
>     at org.gradle.api.tasks.JavaExec.args(JavaExec.java:261)
>     at
> build_7p31187sv6j4incv2nd9fufiog$_run_closure1.doCall(/home/kamils/dev/sandbox/gradle/null_arg/build.gradle:6)|
>
>
>
> We have a PR that proposes just silently ignoring nulls here and in
> other similar cases: https://github.com/gradle/gradle/pull/251
>
> I'm not sure what we want to do here. Silently ignoring nulls doesn't
> sit well with me. I'd prefer we validate the args early and not allow
> nulls. We don't seem to do this anywhere though that I could find.
>
> So:
>
> 1. Is this current situation good enough?
> 2. Should we just silently drop nulls?
> 3. Should we establish patterns/methods for validation and better
> error messages in this case?

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email