[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)

[GitHub] thrift issue #1522: THRIFT-4530: add @Nullable annotations to generated Java...

[jeking3]

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.


---

[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)

[GitHub] thrift issue #1522: THRIFT-4530: add @Nullable annotations to generated Java...

[msridhar]

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


---

[jira] [Commented] (THRIFT-4532) Avoid updating Thrift compiler generated code if the output has not changed

[JIRA]

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

Buğra Gedik commented on THRIFT-4532:
-

We considered two approaches. The first one was just what you described. 
Generate the target file into a temp directory, and if it turns out to be the 
same as the existing one in the output directory, just discard the temporary. 
Otherwise, move it over.

The second one was including a signature into the generated files, like the 
hash of the source thrift file(s) that have contributed to its generation. 
However, given the inclusion support and cross-file referencing, it seems to me 
that a generated file is not necessarily dependent on just one thrift file and 
in the case of mutliple thrift files, using all may cause regenerating 
unnecessarily, as not all entities from an included thrift file would be used.

So it seems like the first approach is more practical, even though it results 
in re-running the code generation logic. In any case, what we care about is the 
generated files, as the most time consuming part is compiling them, especially 
in the case of C++.

 

 

> Avoid updating Thrift compiler generated code if the output has not changed
> ---
>
> Key: THRIFT-4532
> URL: https://issues.apache.org/jira/browse/THRIFT-4532
> Project: Thrift
>  Issue Type: Improvement
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Buğra Gedik
>Priority: Minor
> Fix For: 0.12.0
>
>
> We would like to contribute an improvement to the Thrift compiler that would 
> avoid regenerating target file(s) if they are going to be exactly the same as 
> the ones already present in the same place. This will help when running the 
> Thrift compiler in our build, especially in recursive mode.



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

[jira] [Commented] (THRIFT-4532) Avoid updating Thrift compiler generated code if the output has not changed

[James E. King, III (JIRA)]

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

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


Yes, I agree this would be useful.  I assume the solution involves writing to 
temporary files, then checksumming both, and if not different destroying the 
temporary?

> Avoid updating Thrift compiler generated code if the output has not changed
> ---
>
> Key: THRIFT-4532
> URL: https://issues.apache.org/jira/browse/THRIFT-4532
> Project: Thrift
>  Issue Type: Improvement
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Buğra Gedik
>Priority: Minor
> Fix For: 0.12.0
>
>
> We would like to contribute an improvement to the Thrift compiler that would 
> avoid regenerating target file(s) if they are going to be exactly the same as 
> the ones already present in the same place. This will help when running the 
> Thrift compiler in our build, especially in recursive mode.



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

[GitHub] thrift issue #1522: THRIFT-4530: add @Nullable annotations to generated Java...

[msridhar]

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.


---

[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)

[GitHub] thrift pull request #1522: THRIFT-4530: add @Nullable annotations to generat...

[msridhar]

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




---

[jira] [Commented] (THRIFT-4532) Avoid updating Thrift compiler generated code if the output has not changed

[JIRA]

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

Buğra Gedik commented on THRIFT-4532:
-

[~jking3] If you think this is a valuable contribution, we have Mustafa Cosar 
at Unscrambl, who could contribute this feature. Please let us know.

> Avoid updating Thrift compiler generated code if the output has not changed
> ---
>
> Key: THRIFT-4532
> URL: https://issues.apache.org/jira/browse/THRIFT-4532
> Project: Thrift
>  Issue Type: Improvement
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Buğra Gedik
>Priority: Minor
> Fix For: 0.12.0
>
>
> We would like to contribute an improvement to the Thrift compiler that would 
> avoid regenerating target file(s) if they are going to be exactly the same as 
> the ones already present in the same place. This will help when running the 
> Thrift compiler in our build, especially in recursive mode.



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

[jira] [Updated] (THRIFT-4532) Avoid updating Thrift compiler generated code if the output has not changed

[JIRA]

 [ 
https://issues.apache.org/jira/browse/THRIFT-4532?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Buğra Gedik updated THRIFT-4532:

Description: We would like to contribute an improvement to the Thrift 
compiler that would avoid regenerating target file(s) if they are going to be 
exactly the same as the ones already present in the same place. This will help 
when running the Thrift compiler in our build, especially in recursive mode.  
(was: We would like to contribute an improvement to the Thrift compiler that 
would avoid regenerating target file(s) if they are going to be exactly the 
same as the ones already present in the same place. This will help when running 
the Thrift compiler in our build, especially in recursive mode. It would be 
ideal if we were to avoid overwriting target files that do not have any 
changes.)

> Avoid updating Thrift compiler generated code if the output has not changed
> ---
>
> Key: THRIFT-4532
> URL: https://issues.apache.org/jira/browse/THRIFT-4532
> Project: Thrift
>  Issue Type: Improvement
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Buğra Gedik
>Priority: Minor
> Fix For: 0.12.0
>
>
> We would like to contribute an improvement to the Thrift compiler that would 
> avoid regenerating target file(s) if they are going to be exactly the same as 
> the ones already present in the same place. This will help when running the 
> Thrift compiler in our build, especially in recursive mode.



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

[jira] [Created] (THRIFT-4532) Avoid updating Thrift compiler generated code if the output has not changed

[JIRA]

Buğra Gedik created THRIFT-4532:
---

 Summary: Avoid updating Thrift compiler generated code if the 
output has not changed
 Key: THRIFT-4532
 URL: https://issues.apache.org/jira/browse/THRIFT-4532
 Project: Thrift
  Issue Type: Improvement
  Components: Compiler (General)
Affects Versions: 0.11.0
Reporter: Buğra Gedik
 Fix For: 0.12.0


We would like to contribute an improvement to the Thrift compiler that would 
avoid regenerating target file(s) if they are going to be exactly the same as 
the ones already present in the same place. This will help when running the 
Thrift compiler in our build, especially in recursive mode. It would be ideal 
if we were to avoid overwriting target files that do not have any changes.



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

[jira] [Commented] (THRIFT-4531) Thrift generates wrong Python code for immutable structures with optional members

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-4531:


Github user margars commented on the issue:

https://github.com/apache/thrift/pull/1521
  
Build failed with Gradle POM parsing error which has nothing to do with the 
commit.


> Thrift generates wrong Python code for immutable structures with optional 
> members
> -
>
> Key: THRIFT-4531
> URL: https://issues.apache.org/jira/browse/THRIFT-4531
> Project: Thrift
>  Issue Type: Bug
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Margar Simonyan
>Priority: Major
>
> In order to make generated Python structs hashable one needs to add 
> ( python.immutable; )
> annotation. This is true for Python 3, in Python 2 technically the annotation 
> is not mandatory, however leads to undesirable situation, when equal objects 
> have different hash values.
> If the struct has optional members, then the generated code for read(...) 
> method is wrong and results into undefined local variable error. To fix the 
> issue these variables need to be added to beginning of read(...) method and 
> initialized to None or default values if available. 
> I have a patch for generate\t_py_generator.cc to fix the issue, but I am not 
> familiar with your procedures for contributors.
>  
>  
>  



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

[GitHub] thrift issue #1521: THRIFT-4531 (#1)

[margars]

Github user margars commented on the issue:

https://github.com/apache/thrift/pull/1521
  
Build failed with Gradle POM parsing error which has nothing to do with the 
commit.


---

[jira] [Commented] (THRIFT-4531) Thrift generates wrong Python code for immutable structures with optional members

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-4531:


GitHub user margars opened a pull request:

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

THRIFT-4531 (#1)

THRIFT-4531: Fix generated Python read() method for immutable structs with 
optional members.

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

$ git pull https://github.com/margars/thrift master

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

https://github.com/apache/thrift/pull/1521.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 #1521


commit 969047b3c09af133dd3d24591baedc8ed3c52318
Author: Margar Simonyan 
Date:   2018-03-27T12:30:14Z

THRIFT-4531 (#1)

THRIFT-4531: Fix generated Python read() method for immutable structs with 
optional members.




> Thrift generates wrong Python code for immutable structures with optional 
> members
> -
>
> Key: THRIFT-4531
> URL: https://issues.apache.org/jira/browse/THRIFT-4531
> Project: Thrift
>  Issue Type: Bug
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Margar Simonyan
>Priority: Major
>
> In order to make generated Python structs hashable one needs to add 
> ( python.immutable; )
> annotation. This is true for Python 3, in Python 2 technically the annotation 
> is not mandatory, however leads to undesirable situation, when equal objects 
> have different hash values.
> If the struct has optional members, then the generated code for read(...) 
> method is wrong and results into undefined local variable error. To fix the 
> issue these variables need to be added to beginning of read(...) method and 
> initialized to None or default values if available. 
> I have a patch for generate\t_py_generator.cc to fix the issue, but I am not 
> familiar with your procedures for contributors.
>  
>  
>  



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

[GitHub] thrift pull request #1521: THRIFT-4531 (#1)

[margars]

GitHub user margars opened a pull request:

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

THRIFT-4531 (#1)

THRIFT-4531: Fix generated Python read() method for immutable structs with 
optional members.

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

$ git pull https://github.com/margars/thrift master

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

https://github.com/apache/thrift/pull/1521.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 #1521


commit 969047b3c09af133dd3d24591baedc8ed3c52318
Author: Margar Simonyan 
Date:   2018-03-27T12:30:14Z

THRIFT-4531 (#1)

THRIFT-4531: Fix generated Python read() method for immutable structs with 
optional members.




---

[jira] [Commented] (THRIFT-4531) Thrift generates wrong Python code for immutable structures with optional members

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-4531:


Github user margars commented on the issue:

https://github.com/apache/thrift/pull/1520
  
I am behind corporate firewall and don't know how to squash from the web 
interface, will try from scratch. 


> Thrift generates wrong Python code for immutable structures with optional 
> members
> -
>
> Key: THRIFT-4531
> URL: https://issues.apache.org/jira/browse/THRIFT-4531
> Project: Thrift
>  Issue Type: Bug
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Margar Simonyan
>Priority: Major
>
> In order to make generated Python structs hashable one needs to add 
> ( python.immutable; )
> annotation. This is true for Python 3, in Python 2 technically the annotation 
> is not mandatory, however leads to undesirable situation, when equal objects 
> have different hash values.
> If the struct has optional members, then the generated code for read(...) 
> method is wrong and results into undefined local variable error. To fix the 
> issue these variables need to be added to beginning of read(...) method and 
> initialized to None or default values if available. 
> I have a patch for generate\t_py_generator.cc to fix the issue, but I am not 
> familiar with your procedures for contributors.
>  
>  
>  



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

[GitHub] thrift issue #1520: THRIFT-4531

[margars]

Github user margars commented on the issue:

https://github.com/apache/thrift/pull/1520
  
I am behind corporate firewall and don't know how to squash from the web 
interface, will try from scratch. 


---

[jira] [Commented] (THRIFT-4531) Thrift generates wrong Python code for immutable structures with optional members

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-4531:


Github user margars closed the pull request at:

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


> Thrift generates wrong Python code for immutable structures with optional 
> members
> -
>
> Key: THRIFT-4531
> URL: https://issues.apache.org/jira/browse/THRIFT-4531
> Project: Thrift
>  Issue Type: Bug
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Margar Simonyan
>Priority: Major
>
> In order to make generated Python structs hashable one needs to add 
> ( python.immutable; )
> annotation. This is true for Python 3, in Python 2 technically the annotation 
> is not mandatory, however leads to undesirable situation, when equal objects 
> have different hash values.
> If the struct has optional members, then the generated code for read(...) 
> method is wrong and results into undefined local variable error. To fix the 
> issue these variables need to be added to beginning of read(...) method and 
> initialized to None or default values if available. 
> I have a patch for generate\t_py_generator.cc to fix the issue, but I am not 
> familiar with your procedures for contributors.
>  
>  
>  



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

[GitHub] thrift pull request #1520: THRIFT-4531

[margars]

Github user margars closed the pull request at:

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


---

[jira] [Commented] (THRIFT-4531) Thrift generates wrong Python code for immutable structures with optional members

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-4531:


Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1520
  
Also, please squash your commits.


> Thrift generates wrong Python code for immutable structures with optional 
> members
> -
>
> Key: THRIFT-4531
> URL: https://issues.apache.org/jira/browse/THRIFT-4531
> Project: Thrift
>  Issue Type: Bug
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Margar Simonyan
>Priority: Major
>
> In order to make generated Python structs hashable one needs to add 
> ( python.immutable; )
> annotation. This is true for Python 3, in Python 2 technically the annotation 
> is not mandatory, however leads to undesirable situation, when equal objects 
> have different hash values.
> If the struct has optional members, then the generated code for read(...) 
> method is wrong and results into undefined local variable error. To fix the 
> issue these variables need to be added to beginning of read(...) method and 
> initialized to None or default values if available. 
> I have a patch for generate\t_py_generator.cc to fix the issue, but I am not 
> familiar with your procedures for contributors.
>  
>  
>  



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

[GitHub] thrift issue #1520: THRIFT-4531

[dcelasun]

Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1520
  
Also, please squash your commits.


---

[jira] [Issue Comment Deleted] (THRIFT-4531) Thrift generates wrong Python code for immutable structures with optional members

[Margar Simonyan (JIRA)]

 [ 
https://issues.apache.org/jira/browse/THRIFT-4531?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Margar Simonyan updated THRIFT-4531:

Comment: was deleted

(was: Added pull request https://github.com/apache/thrift/pull/1520 )

> Thrift generates wrong Python code for immutable structures with optional 
> members
> -
>
> Key: THRIFT-4531
> URL: https://issues.apache.org/jira/browse/THRIFT-4531
> Project: Thrift
>  Issue Type: Bug
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Margar Simonyan
>Priority: Major
>
> In order to make generated Python structs hashable one needs to add 
> ( python.immutable; )
> annotation. This is true for Python 3, in Python 2 technically the annotation 
> is not mandatory, however leads to undesirable situation, when equal objects 
> have different hash values.
> If the struct has optional members, then the generated code for read(...) 
> method is wrong and results into undefined local variable error. To fix the 
> issue these variables need to be added to beginning of read(...) method and 
> initialized to None or default values if available. 
> I have a patch for generate\t_py_generator.cc to fix the issue, but I am not 
> familiar with your procedures for contributors.
>  
>  
>  



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

[jira] [Commented] (THRIFT-4531) Thrift generates wrong Python code for immutable structures with optional members

[Margar Simonyan (JIRA)]

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

Margar Simonyan commented on THRIFT-4531:
-

Added pull request https://github.com/apache/thrift/pull/1520 

> Thrift generates wrong Python code for immutable structures with optional 
> members
> -
>
> Key: THRIFT-4531
> URL: https://issues.apache.org/jira/browse/THRIFT-4531
> Project: Thrift
>  Issue Type: Bug
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Margar Simonyan
>Priority: Major
>
> In order to make generated Python structs hashable one needs to add 
> ( python.immutable; )
> annotation. This is true for Python 3, in Python 2 technically the annotation 
> is not mandatory, however leads to undesirable situation, when equal objects 
> have different hash values.
> If the struct has optional members, then the generated code for read(...) 
> method is wrong and results into undefined local variable error. To fix the 
> issue these variables need to be added to beginning of read(...) method and 
> initialized to None or default values if available. 
> I have a patch for generate\t_py_generator.cc to fix the issue, but I am not 
> familiar with your procedures for contributors.
>  
>  
>  



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

[jira] [Commented] (THRIFT-4531) Thrift generates wrong Python code for immutable structures with optional members

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-4531:


Github user margars commented on the issue:

https://github.com/apache/thrift/pull/1520
  
Sorry, I am not familiar with all this setup and rules.


> Thrift generates wrong Python code for immutable structures with optional 
> members
> -
>
> Key: THRIFT-4531
> URL: https://issues.apache.org/jira/browse/THRIFT-4531
> Project: Thrift
>  Issue Type: Bug
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Margar Simonyan
>Priority: Major
>
> In order to make generated Python structs hashable one needs to add 
> ( python.immutable; )
> annotation. This is true for Python 3, in Python 2 technically the annotation 
> is not mandatory, however leads to undesirable situation, when equal objects 
> have different hash values.
> If the struct has optional members, then the generated code for read(...) 
> method is wrong and results into undefined local variable error. To fix the 
> issue these variables need to be added to beginning of read(...) method and 
> initialized to None or default values if available. 
> I have a patch for generate\t_py_generator.cc to fix the issue, but I am not 
> familiar with your procedures for contributors.
>  
>  
>  



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

[GitHub] thrift issue #1520: THRIFT-4531

[margars]

Github user margars commented on the issue:

https://github.com/apache/thrift/pull/1520
  
Sorry, I am not familiar with all this setup and rules.


---

[jira] [Commented] (THRIFT-4531) Thrift generates wrong Python code for immutable structures with optional members

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-4531:


Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1520
  
There are no changes in this PR. Please correct it and force push.


> Thrift generates wrong Python code for immutable structures with optional 
> members
> -
>
> Key: THRIFT-4531
> URL: https://issues.apache.org/jira/browse/THRIFT-4531
> Project: Thrift
>  Issue Type: Bug
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Margar Simonyan
>Priority: Major
>
> In order to make generated Python structs hashable one needs to add 
> ( python.immutable; )
> annotation. This is true for Python 3, in Python 2 technically the annotation 
> is not mandatory, however leads to undesirable situation, when equal objects 
> have different hash values.
> If the struct has optional members, then the generated code for read(...) 
> method is wrong and results into undefined local variable error. To fix the 
> issue these variables need to be added to beginning of read(...) method and 
> initialized to None or default values if available. 
> I have a patch for generate\t_py_generator.cc to fix the issue, but I am not 
> familiar with your procedures for contributors.
>  
>  
>  



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

[GitHub] thrift issue #1520: THRIFT-4531

[dcelasun]

Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1520
  
There are no changes in this PR. Please correct it and force push.


---

[jira] [Commented] (THRIFT-4531) Thrift generates wrong Python code for immutable structures with optional members

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on THRIFT-4531:


GitHub user margars opened a pull request:

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

THRIFT-4531

I am not familiar with your rules for contributions. This issue is 
described in JIRA THRIFT-4531. 

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

$ git pull https://github.com/margars/thrift master

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

https://github.com/apache/thrift/pull/1520.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 #1520


commit d682b13e5e7672702e53f1e696263ef5293d8d37
Author: Margar Simonyan 
Date:   2018-03-27T11:22:10Z

Update t_py_generator.cc

Fix the generated read() method for immutable structures with optional 
members.

commit 14a4c46c480ae041bb51397cefc9be575f17f79a
Author: Margar Simonyan 
Date:   2018-03-27T11:26:51Z

Merge pull request #1 from margars/THRIFT-4531

Update t_py_generator.cc

commit 29289f4d8c88fee636b779a94aeae48bf81671b0
Author: Margar Simonyan 
Date:   2018-03-27T11:28:11Z

Revert "Update t_py_generator.cc"

commit 56dc4f04c20a9601063deec75630ff4212937a46
Author: Margar Simonyan 
Date:   2018-03-27T11:28:35Z

Merge pull request #2 from margars/revert-1-THRIFT-4531

Revert "Update t_py_generator.cc"




> Thrift generates wrong Python code for immutable structures with optional 
> members
> -
>
> Key: THRIFT-4531
> URL: https://issues.apache.org/jira/browse/THRIFT-4531
> Project: Thrift
>  Issue Type: Bug
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Margar Simonyan
>Priority: Major
>
> In order to make generated Python structs hashable one needs to add 
> ( python.immutable; )
> annotation. This is true for Python 3, in Python 2 technically the annotation 
> is not mandatory, however leads to undesirable situation, when equal objects 
> have different hash values.
> If the struct has optional members, then the generated code for read(...) 
> method is wrong and results into undefined local variable error. To fix the 
> issue these variables need to be added to beginning of read(...) method and 
> initialized to None or default values if available. 
> I have a patch for generate\t_py_generator.cc to fix the issue, but I am not 
> familiar with your procedures for contributors.
>  
>  
>  



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

[GitHub] thrift pull request #1520: THRIFT-4531

[margars]

GitHub user margars opened a pull request:

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

THRIFT-4531

I am not familiar with your rules for contributions. This issue is 
described in JIRA THRIFT-4531. 

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

$ git pull https://github.com/margars/thrift master

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

https://github.com/apache/thrift/pull/1520.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 #1520


commit d682b13e5e7672702e53f1e696263ef5293d8d37
Author: Margar Simonyan 
Date:   2018-03-27T11:22:10Z

Update t_py_generator.cc

Fix the generated read() method for immutable structures with optional 
members.

commit 14a4c46c480ae041bb51397cefc9be575f17f79a
Author: Margar Simonyan 
Date:   2018-03-27T11:26:51Z

Merge pull request #1 from margars/THRIFT-4531

Update t_py_generator.cc

commit 29289f4d8c88fee636b779a94aeae48bf81671b0
Author: Margar Simonyan 
Date:   2018-03-27T11:28:11Z

Revert "Update t_py_generator.cc"

commit 56dc4f04c20a9601063deec75630ff4212937a46
Author: Margar Simonyan 
Date:   2018-03-27T11:28:35Z

Merge pull request #2 from margars/revert-1-THRIFT-4531

Revert "Update t_py_generator.cc"




---

[jira] [Commented] (THRIFT-4531) Thrift generates wrong Python code for immutable structures with optional members

[Can Celasun (JIRA)]

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

Can Celasun commented on THRIFT-4531:
-

See the [contributing guide|http://thrift.apache.org/docs/HowToContribute] for 
submitting pull requests.

> Thrift generates wrong Python code for immutable structures with optional 
> members
> -
>
> Key: THRIFT-4531
> URL: https://issues.apache.org/jira/browse/THRIFT-4531
> Project: Thrift
>  Issue Type: Bug
>  Components: Compiler (General)
>Affects Versions: 0.11.0
>Reporter: Margar Simonyan
>Priority: Major
>
> In order to make generated Python structs hashable one needs to add 
> ( python.immutable; )
> annotation. This is true for Python 3, in Python 2 technically the annotation 
> is not mandatory, however leads to undesirable situation, when equal objects 
> have different hash values.
> If the struct has optional members, then the generated code for read(...) 
> method is wrong and results into undefined local variable error. To fix the 
> issue these variables need to be added to beginning of read(...) method and 
> initialized to None or default values if available. 
> I have a patch for generate\t_py_generator.cc to fix the issue, but I am not 
> familiar with your procedures for contributors.
>  
>  
>  



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

[jira] [Created] (THRIFT-4531) Thrift generates wrong Python code for immutable structures with optional members

[Margar Simonyan (JIRA)]

Margar Simonyan created THRIFT-4531:
---

 Summary: Thrift generates wrong Python code for immutable 
structures with optional members
 Key: THRIFT-4531
 URL: https://issues.apache.org/jira/browse/THRIFT-4531
 Project: Thrift
  Issue Type: Bug
  Components: Compiler (General)
Affects Versions: 0.11.0
Reporter: Margar Simonyan


In order to make generated Python structs hashable one needs to add 

( python.immutable; )

annotation. This is true for Python 3, in Python 2 technically the annotation 
is not mandatory, however leads to undesirable situation, when equal objects 
have different hash values.

If the struct has optional members, then the generated code for read(...) 
method is wrong and results into undefined local variable error. To fix the 
issue these variables need to be added to beginning of read(...) method and 
initialized to None or default values if available. 

I have a patch for generate\t_py_generator.cc to fix the issue, but I am not 
familiar with your procedures for contributors.

 

 

 



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