[Drift] Allow adding custom thrift type when using generator #112
Conversation
shangm2
force-pushed
the
customeThriftType
branch
4 times, most recently
from
b3a1423 to
0c13a86
Compare
shangm2
changed the title
shangm2
force-pushed
the
customeThriftType
branch
2 times, most recently
from
9b2a65c to
06ea0c1
Compare
| <artifactId>maven-dependency-plugin</artifactId> | ||
| <configuration> | ||
| <ignoredNonTestScopedDependencies> | ||
| <dependency>com.facebook.airlift:units</dependency> |
There was a problem hiding this comment.
Why does it need to be ignored?
|
|
||
| public void addKnowTypes(ThriftType type) | ||
| { | ||
| BUILT_IN_TYPES.add(type); |
There was a problem hiding this comment.
Is it necessary to update BUILT_IN_TYPES?
There was a problem hiding this comment.
There was a problem hiding this comment.
Hmm, I see.
Modifying the static map of built in types may result in unexpected side effects. Consider an application using the ThriftIdlGenerator class to generate multiple unrelated IDLs.
Maybe keep the static BUILT_IN_TYPES map immutable and add one more, non static, customTypes map and then initialize the knownTypes map using both, the static BUILT_IN_TYPES and the customTypes map.
You may want to rename the addKnownType method addCustomType
There was a problem hiding this comment.
Good point. Thanks for the suggestion! Code updated.
| this.recursive = config.isRecursive(); | ||
| } | ||
|
|
||
| public void addKnowTypes(ThriftType type) |
|
|
||
| <dependency> | ||
| <groupId>joda-time</groupId> | ||
| <artifactId>joda-time</artifactId> |
There was a problem hiding this comment.
looks like this is used only for Tests ? if so can we make the scope as test ?
There was a problem hiding this comment.
| <dependency> | ||
| <groupId>joda-time</groupId> | ||
| <artifactId>joda-time</artifactId> | ||
| <scope>test</scope> |
4 participants
Uh oh!
There was an error while loading. Please reload this page.