Generate code for enums in definition blocks #31
Conversation
Eric-Warehime
changed the title
| @@ -291,6 +291,9 @@ public void terminate() { | |||
| public void onEvent(Publisher.Events event, TypeDef type) { | |||
| switch(event) { | |||
| case NEW_PROPERTY: | |||
| if (type.isOfType("enum")) { | |||
There was a problem hiding this comment.
I'm not sure about this, could we change the go templates to skip over enums?
Ideally we would use the template approach for all SDKs, so at least the JavaSDK would need this event.
There was a problem hiding this comment.
Wouldn't the go SDK still need this, but use "string"?
There was a problem hiding this comment.
Ideally we would use the template approach for all SDKs, so at least the JavaSDK would need this event.
I see your point...I'll take a look and see if it's easy to configure enum-specific behavior in the templates.
From what I can tell both the JS and Go Sdks just use string fields for enums so there is no need to take an action in addition to those we're already taking when generating paths, models, etc. like we are in Java.
There was a problem hiding this comment.
@Eric-Warehime Approving based on the evidence provided by downstream PRs generating expected diffs. Thank you for tackling the problem and for finding a way to restore the code generation process! ☕
Aside with a few caveats: Perhaps in a future iteration, tests can be added here to defend against regressions and/or behavior changes.
- Since we have proof the changes work, I don't think the remark ought to hold up the PR.
- As a reviewer, I feel obligated to consider testing in my review. Admittedly, I am unfamiliar with the repo's conventions + practices. And a cursory review of recent PRs does not reveal tests.
| @@ -118,6 +118,9 @@ public void onEvent(Events event, TypeDef type) { | |||
| case BODY_CONTENT: | |||
| javaQueryWriter.addQueryProperty(type, false, false, true); | |||
| break; | |||
| case ENUM_DEFINITION: | |||
| this.storeEnumDefinition(type); | |||
There was a problem hiding this comment.
On closer inspection, I think we can add this check without changing the parser. This would go in the TypeDef event:
case NEW_PROPERTY:
javaModelWriter.newProperty(type);
+ if (type.enumValues != null && type.enumValues.size() > 0) {
+ this.storeEnumDefinition(type);
+ }There was a problem hiding this comment.
I don't think this will work. The reason we have to change the parser is because generateAlgodIndexerObjects is the only method which is looking at the definitions block, and the following code block ensure that objects in that block are skipped if they don't have properties defined, which enums do not have.
if (!hasProperties(cls.getValue())) {
// If it has no properties, no class is needed for this type.
continue;
}
So in that case we wouldn't ever enter the NEW_PROPERTY event block.
There was a problem hiding this comment.
Also, as an aside, enums implicity defined in the Parameters section are omitted from the generated code (Format)
This updates the generator to address #19 --it generates enums in the
Enums.javafile which include any enums listed in the definitions section. The template generator was also updated to ignore these since the go sdk generated using the template generator does not have a concept of enums really.