facebookarchive / swift

An annotation-based Java library for creating Thrift serializable types and services.
Apache License 2.0
900 stars 297 forks source link

add code generator and maven plugin #27

Closed hgschmie closed 11 years ago

hgschmie commented 11 years ago

do "mvn clean install", then "cd swift-demo; mvn clean package"

Needs some polishing and logging in the generator and the maven plugin but generally works. The template code also requires a bit more looking after.

TODO

usage:

 <plugin>
    <groupId>com.facebook.mojo</groupId>
    <artifactId>swift-maven-plugin</artifactId>
    <version>0.1-SNAPSHOT</version>
    <executions>
        <execution>
        <goals>
                <goal>generate</goal>
            </goals>
        </execution>
    </executions>
    <configuration>
        <!-- optional, default to values from idl -->
        <packageName>com.fb.demo</packageName>
        <!-- optional, this is the default -->
        <outputFolder>${project.basedir}/target/generated-sources/swift</outputFolder>
        <idlFiles>
            <directory>${project.basedir}/src/main/idl</directory>
            <includes>
                <include>**/*.thrift</include>
            </includes>
        </idlFiles>
    </configuration>
</plugin>
andrewcox commented 11 years ago

I have a code generator using ANTLR's StringTemplate (doesn't handle constants)... Perhaps we should compare?

hgschmie commented 11 years ago

Sounds good. The code generator here uses stringtemplate, too (see src/main/resources/template/java/regular.st). I am planning to add additional templates that generate e.g. immutable beans.

andrewcox commented 11 years ago

Once you've addressed the above stuff, I'd say go ahead and merge, since this doesn't touch other swift functionality it seems safe to go with it now, and we can fix/improve it after.

jaxlaw commented 11 years ago

feature request : can we also handle typedef properly ?

e.g. in Maestro.thrift

typedef i32 OdsAdHocAggType

the generated classes are not compiling because OdsAdHocAggType is unresolved.

or things like

typedef string fbstring

also breaks the generated code.

jaxlaw commented 11 years ago

also, it seems that if A.thrift includes B.thrift and C.thrift , and B.thrift includes C.thrift , the generator will bork about duplicate definitions in C.thrift

jaxlaw commented 11 years ago

2 more issues :

  1. if we don't specify the enum code in the IDL, the generated java enum code sets all enum values to 0.
  2. optional primitive values defined in a thrift struct should be generated as boxed type to allow for null, otherwise they will have default values and services interpreting the thrift struct may have issue.
jaxlaw commented 11 years ago

also, would be nice to handle default values in thrift struct

andrewcox commented 11 years ago

Yeah, my take on the generator (in one of the branches on my fork) does handle this (though there are other things this version does that mine does not). In particular if you specify some values and not others, Java afaict does not do the same thing as C (Respect explicitly assigned values, +1 for each successive non-explicitly assigned enum value) so I think we need to do it that way.

From: Jax Law notifications@github.com<mailto:notifications@github.com> Reply-To: facebook/swift reply@reply.github.com<mailto:reply@reply.github.com> Date: Wed, 7 Nov 2012 23:03:46 -0800 To: facebook/swift swift@noreply.github.com<mailto:swift@noreply.github.com> Cc: Andrew Cox andrewcox@fb.com<mailto:andrewcox@fb.com> Subject: Re: [swift] add code generator and maven plugin (#27)

2 more issues :

  1. if we don't specify the enum code in the IDL, the generated java enum code sets all enum values to 0.
  2. optional primitive values defined in a thrift struct should be generated as boxed type to allow for null, otherwise they will have default values and services interpreting the thrift struct may have issue.

— Reply to this email directly or view it on GitHubhttps://github.com/facebook/swift/pull/27#issuecomment-10178461.

hgschmie commented 11 years ago

@jaxlaw, thanks for the thorough feedback, when I find time, I will add this. I did not know that typedefs are a non-C thing.