-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42748/#review117164
-----------------------------------------------------------



This is just a quick first pass review where I checked for overall organization 
of the code and structure. I was able to follow along and understand the 
organization just fine. After spending time reading this carefully, I now 
better understand the possible concern of the maintenance burden of this much 
non-trival code. A subsequent review of this will have me focusing on the 
implementation of each visitor and codegen.

I have left some comments for small things that stood out to me as I was 
reviewing.


buildSrc/build.gradle (line 21)
<https://reviews.apache.org/r/42748/#comment178244>

    This `compileJava` clause seems to be uncessary. I have been able to remove 
it and have the project compile successfully.



buildSrc/build.gradle (line 56)
<https://reviews.apache.org/r/42748/#comment178245>

    It seems to me that the `guava`, `libthrift`, `slf4j` and `junit` versions 
should stay in sync with the ones defined in `build.gradle` in the root.
    
    Do you know if it is possible to pass down variables from build.gradle into 
this script? If not, would you mind adding comments both in `build.gradle` and 
here telling us that they should be kept in sync?



buildSrc/build.gradle (line 65)
<https://reviews.apache.org/r/42748/#comment178246>

    The SLF4J dependency seems like overkill here, is there a reason why the 
thrift generation code doesn't just use `j.u.logging`?



buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftAnnotations.java
 (line 37)
<https://reviews.apache.org/r/42748/#comment178247>

    Perhaps this should be a `List<ThriftAnnotation>` than an array?



buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/ThriftGen.java 
(line 159)
<https://reviews.apache.org/r/42748/#comment178248>

    I think the `PathCharSource` could just be replaced with 
`com.google.common.io.Files#asCharSource` where you call `toFile` on 
`thriftFile`.
    
    No need to create another class here.


- Zameer Manji


On Jan. 26, 2016, 9:16 p.m., John Sirois wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42748/
> -----------------------------------------------------------
> 
> (Updated Jan. 26, 2016, 9:16 p.m.)
> 
> 
> Review request for Aurora, Bill Farner and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This generator emits immutable objects and carries over thrift
> annotations to java annotations for both subsequent build-time
> code generation via annotation processors and for runtime use
> in interceptors and other reflective tools.
> 
>  .gitignore                                                                   
>                                       |   3 +
>  build.gradle                                                                 
>                                       |  47 ++-
>  buildSrc/build.gradle                                                        
>                                       |  61 ++++
>  settings.gradle => buildSrc/settings.gradle                                  
>                                       |   3 +-
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftAnnotation.java
>                                     |  45 +++
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftAnnotations.java
>                                    |  38 +++
>  buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftEntity.java  
>                                       |  70 ++++
>  buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftFields.java  
>                                       |  65 ++++
>  buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftService.java 
>                                       |  43 +++
>  buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftStruct.java  
>                                       |  80 +++++
>  buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftUnion.java   
>                                       |  76 +++++
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/AbstractStructRenderer.java
>                         | 132 ++++++++
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/BaseEmitter.java
>                                    | 143 +++++++++
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/BaseVisitor.java
>                                    | 533 +++++++++++++++++++++++++++++++
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/ConstVisitor.java
>                                   |  72 +++++
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/IntegerEnumVisitor.java
>                             | 102 ++++++
>  buildSrc/{build.gradle => 
> thriftGen/src/main/java/org/apache/aurora/thrift/build/ParseException.java}   
>            |  15 +-
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/ServiceVisitor.java
>                                 | 279 ++++++++++++++++
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/StructVisitor.java
>                                  | 619 +++++++++++++++++++++++++++++++++++
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/SymbolTable.java
>                                    | 180 +++++++++++
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/ThriftGen.java
>                                      | 190 +++++++++++
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/ThriftGenVisitor.java
>                               | 122 +++++++
>  buildSrc/{build.gradle => 
> thriftGen/src/main/java/org/apache/aurora/thrift/build/UnexpectedTypeException.java}
>      |  13 +-
>  
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/UnionVisitor.java
>                                   | 254 +++++++++++++++
>  buildSrc/{build.gradle => 
> thriftGen/src/main/java/org/apache/aurora/thrift/build/UnsupportedFeatureException.java}
>  |  12 +-
>  buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/Visitor.java 
>                                       |  95 ++++++
>  
> buildSrc/thriftGen/src/test/java/org/apache/aurora/thrift/build/ThriftGenTest.java
>                                  | 869 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  settings.gradle                                                              
>                                       |   2 +-
>  28 files changed, 4127 insertions(+), 36 deletions(-)
> 
> 
> Diffs
> -----
> 
>   .gitignore 1af09a251b3f76c13813033d32aa7efba9aef304 
>   build.gradle 5b9e0152bfe3fe1f304fa839cfc4cec646819c2e 
>   buildSrc/build.gradle e3d6debf1959ec2c50970c317b59a04a2d9c1f82 
>   buildSrc/settings.gradle PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftAnnotation.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftAnnotations.java
>  PRE-CREATION 
>   buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftEntity.java 
> PRE-CREATION 
>   buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftFields.java 
> PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftService.java 
> PRE-CREATION 
>   buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftStruct.java 
> PRE-CREATION 
>   buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftUnion.java 
> PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/AbstractStructRenderer.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/BaseEmitter.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/BaseVisitor.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/ConstVisitor.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/IntegerEnumVisitor.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/ParseException.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/ServiceVisitor.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/StructVisitor.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/SymbolTable.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/ThriftGen.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/ThriftGenVisitor.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/UnexpectedTypeException.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/UnionVisitor.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/UnsupportedFeatureException.java
>  PRE-CREATION 
>   
> buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/Visitor.java 
> PRE-CREATION 
>   
> buildSrc/thriftGen/src/test/java/org/apache/aurora/thrift/build/ThriftGenTest.java
>  PRE-CREATION 
>   settings.gradle b097e2fd958fa0ce6076fc104eb3890c4029295d 
> 
> Diff: https://reviews.apache.org/r/42748/diff/
> 
> 
> Testing
> -------
> 
> Locally green: `./gradlew -Pq test`
> 
> 
> Thanks,
> 
> John Sirois
> 
>

Reply via email to