> On Jan. 31, 2016, 6:13 p.m., Zameer Manji wrote:
> > 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.

Thanks for looking, but I'm going to close this review series out as discarded 
and make that more clear on the PROPOSAL thread since consensus has settled 
there on working through Apache Thrift 1st, and that will move the work done 
here in 1/3 over to Thrift.


> On Jan. 31, 2016, 6:13 p.m., Zameer Manji wrote:
> > buildSrc/build.gradle, line 56
> > <https://reviews.apache.org/r/42748/diff/1/?file=1219830#file1219830line56>
> >
> >     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?

Yeah - passing variables down definitely works, I have a TODO for exactly this 
in my branch for 3/3 https://reviews.apache.org/r/42756/diff/2?page=1#5 line 
36.  This file deserves the same treatment and TODO though.


> On Jan. 31, 2016, 6:13 p.m., Zameer Manji wrote:
> > buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/ThriftAnnotations.java,
> >  line 37
> > <https://reviews.apache.org/r/42748/diff/1/?file=1219833#file1219833line37>
> >
> >     Perhaps this should be a `List<ThriftAnnotation>` than an array?

Annotation types can only have - for some definition of primitive - primitive 
typed values.  Arrays, other annotation types, enums and Class instances are 
the oddest of these primitive types


> On Jan. 31, 2016, 6:13 p.m., Zameer Manji wrote:
> > buildSrc/thriftGen/src/main/java/org/apache/aurora/thrift/build/ThriftGen.java,
> >  line 159
> > <https://reviews.apache.org/r/42748/diff/1/?file=1219848#file1219848line159>
> >
> >     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.

Nack - Path.toFile works for the native FS layer provided by the JDK but is an 
optional operation and - importantly - not one supported by Jimfs which is used 
in unit tests.


> On Jan. 31, 2016, 6:13 p.m., Zameer Manji wrote:
> > buildSrc/build.gradle, line 65
> > <https://reviews.apache.org/r/42748/diff/1/?file=1219830#file1219830line65>
> >
> >     The SLF4J dependency seems like overkill here, is there a reason why 
> > the thrift generation code doesn't just use `j.u.logging`?

Agreed - this dep is un-needed.


> On Jan. 31, 2016, 6:13 p.m., Zameer Manji wrote:
> > buildSrc/build.gradle, line 21
> > <https://reviews.apache.org/r/42748/diff/1/?file=1219830#file1219830line21>
> >
> >     This `compileJava` clause seems to be uncessary. I have been able to 
> > remove it and have the project compile successfully.

You're right - although the proof of the pudding is a failed compile for, say 
`JAVA_HOME=/usr/lib/jvm/java-7-openjdk ./gradlew...`.  ThriftGen was top-level 
in buildSrc when I added this and it now can be removed since the `subprojects 
{...}` now handles the check.


- John


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


On Jan. 26, 2016, 10: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, 10: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