[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16418942#comment-16418942
 ] 

ASF GitHub Bot commented on THRIFT-4530:


Github user asfgit closed the pull request at:

https://github.com/apache/thrift/pull/1522


> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16418924#comment-16418924
 ] 

ASF GitHub Bot commented on THRIFT-4530:


Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1522
  
That is an intermittent test failure that I thought I resolved.  :)  it is 
C++ only


> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16418176#comment-16418176
 ] 

ASF GitHub Bot commented on THRIFT-4530:


Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1522
  
I don't think it's relevant either, seems like a flaky test. I think we are 
good to merge.

@jeking3 any idea on the failing appveyor test, have you seen it before? 
I'm inclined to ignore it unless you say otherwise.


> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16418132#comment-16418132
 ] 

ASF GitHub Bot commented on THRIFT-4530:


Github user msridhar commented on the issue:

https://github.com/apache/thrift/pull/1522
  
Hrm, now I see a concurrency test failure on appveyor.  Doesn't seem 
related to my change, though I'm not 100% sure.


> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16417654#comment-16417654
 ] 

ASF GitHub Bot commented on THRIFT-4530:


Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1522
  
No need, I'll squash once Travis is all green.


> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16417645#comment-16417645
 ] 

ASF GitHub Bot commented on THRIFT-4530:


Github user msridhar commented on the issue:

https://github.com/apache/thrift/pull/1522
  
I just pushed up another commit with the renaming.  I assume you can do a 
squash and merge, but if you'd like me to squash just let me know.


> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16417221#comment-16417221
 ] 

ASF GitHub Bot commented on THRIFT-4530:


Github user jeking3 commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1522#discussion_r177720513
  
--- Diff: lib/java/src/org/apache/thrift/annotations/Nullable.java ---
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.thrift.annotations;
--- End diff --

Any reason why java language uses "annotation" but you selected 
"annotations" for thrift?  Seems like we should be using "annotation".


> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16416594#comment-16416594
 ] 

ASF GitHub Bot commented on THRIFT-4530:


Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1522
  
Yes there is an issue that appears in builds where lisp cannot find a file 
it just supposedly wrote.  Lisp is new to the project, this is an ongoing thing.


> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16416581#comment-16416581
 ] 

ASF GitHub Bot commented on THRIFT-4530:


Github user msridhar commented on the issue:

https://github.com/apache/thrift/pull/1522
  
Looks like the Travis failure is on Common Lisp codegen 
(https://api.travis-ci.org/v3/job/359136811/log.txt), which has nothing to do 
with this PR


> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16416443#comment-16416443
 ] 

ASF GitHub Bot commented on THRIFT-4530:


Github user msridhar commented on the issue:

https://github.com/apache/thrift/pull/1522
  
Note that while there are no new tests, the code under 
`tutorial/java/gen-java` includes the annotations and compiles without issue.  
Since this change doesn't affect runtime behavior of the generated code I think 
this might be sufficient.


> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16416437#comment-16416437
 ] 

ASF GitHub Bot commented on THRIFT-4530:


GitHub user msridhar opened a pull request:

https://github.com/apache/thrift/pull/1522

THRIFT-4530: add @Nullable annotations to generated Java code

Use our own `org.apache.thrift.annotations.Nullable` annotation to avoid
introducing a third-party dependency

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/msridhar/thrift THRIFT-4530

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/1522.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1522


commit 6bc560a1a55a1fca720b4fe7057f25c06fc358d1
Author: Manu Sridharan 
Date:   2018-03-20T21:45:22Z

THRIFT-4530: add @Nullable annotations to generated Java code

Use our own `org.apache.thrift.annotations.Nullable` type to avoid
introducing a third-party dependency




> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[James E. King, III (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16413981#comment-16413981
 ] 

James E. King, III commented on THRIFT-4530:


If nullable annotations have no other side-effects why don't we just add the 
annotation and then at such time someone finds a need to disable it they can 
add the code necessary to do no?

> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[Can Celasun (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16412550#comment-16412550
 ] 

Can Celasun commented on THRIFT-4530:
-

I think it's better if they are on by default, but if anyone has objections I'm 
happy with off as well.

cc [~jking3]

> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[Manu Sridharan (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16412392#comment-16412392
 ] 

Manu Sridharan commented on THRIFT-4530:


Great!  I will definitely post the PR on GitHub. Do we want {{@Nullable}} 
annotations off by default, with a compiler flag to enable them?  Or should 
they just be on by default?  In any case, hope to have a PR up soon.  Thanks!

> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (THRIFT-4530) proposal: add nullability annotations to generated Java code

[Can Celasun (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4530?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16412173#comment-16412173
 ] 

Can Celasun commented on THRIFT-4530:
-

I think it's a good idea. Regarding which annotation to use, I think we should 
definitely include a default {{@Nullable}} implementation, but it might also be 
worth adding a compiler flag to use a different one.

For the patch, it's better if you send it as a PR on Github instead of posting 
it here, as a PR will trigger a Travis build.

> proposal: add nullability annotations to generated Java code
> 
>
> Key: THRIFT-4530
> URL: https://issues.apache.org/jira/browse/THRIFT-4530
> Project: Thrift
>  Issue Type: New Feature
>  Components: Java - Compiler
>Reporter: Manu Sridharan
>Priority: Major
>
> I'd like to propose (optionally) including {{@Nullable}} annotations in 
> Thrift-generated Java code.  I'm the main author of NullAway 
> ([https://github.com/uber/NullAway)] and we'd like to better support users 
> who are using Thrift.  The change would involve changing the Java code 
> generator to include {{@Nullable}} annotations on every field, method return 
> value, and method parameter in the public API of generated code that may be 
> null.  With these annotations, NullAway users will get warnings when their 
> client code is missing appropriate null checks.  Also, IDEs like IntelliJ 
> will give better warnings about missing null checks.  As part of this change, 
> I would also add support to NullAway for understanding {{isSetX()}} methods 
> to avoid excessive false positives.
> Regarding which {{@Nullable}} annotation to use, Thrift seems to try to 
> minimize third-party dependencies, but we could simply include a new Thrift 
> {{@Nullable}} annotation, and it will work fine with NullAway and most other 
> tools.
> I have a WIP patch to generate these annotations, but I wanted to get 
> feedback from the maintainers before opening a PR.  We could of course make 
> the annotation generation optional and default it to being off, if desired.  
> Anyway, thoughts / feedback welcome.  Thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)