-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
[go-experimental] Fix multiple go compilation errors and enable go integration test #5075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
685a7af
31527ca
9370937
d665d7c
48526da
262dc8a
a3bb335
735f84c
53226d3
be0b85c
295bbb7
e2cf635
1769234
1ed28d8
705a8c3
f115d8c
220e0cf
2ac6769
d2e34ee
0049c79
60e6709
4cd66c2
1f0676c
ef25e55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,7 +71,6 @@ public void processOpts() { | |
|
|
||
| @Override | ||
| public Map<String, Object> postProcessModels(Map<String, Object> objs) { | ||
| objs = super.postProcessModels(objs); | ||
|
|
||
| List<Map<String, Object>> models = (List<Map<String, Object>>) objs.get("models"); | ||
| for (Map<String, Object> m : models) { | ||
|
|
@@ -83,16 +82,27 @@ public Map<String, Object> postProcessModels(Map<String, Object> objs) { | |
| } | ||
|
|
||
| for (CodegenProperty param : model.vars) { | ||
| if (!param.isNullable) { | ||
| if (!param.isNullable || param.isMapContainer || param.isListContainer) { | ||
| continue; | ||
| } | ||
|
|
||
| param.dataType = "Nullable" + Character.toUpperCase(param.dataType.charAt(0)) | ||
| if (param.isDateTime) { | ||
| // Note this could have been done by adding the following line in processOpts(), | ||
| // however, we only want to represent the DateTime object as NullableTime if | ||
| // it's marked as nullable in the spec. | ||
| // typeMapping.put("DateTime", "NullableTime"); | ||
| param.dataType = "NullableTime"; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not invoke the following code in go-experimental processOpts() instead of modifying the dataType here? What is the reason for changing the type in postProcessModels()?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed with using typeMapping to directly map the type instead of using the code above.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So interestingly, adding typeMapping.put("DateTime", "NullableTime") does not produce the same output for the samples. I wonder if that was done on purpose. When the dataType is set in postProcessModels(), it does not have the same effect as when typeMapping is invoked in processOpts(). Is that done on purpose?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only want to represent the DateTime object as NullableTime if it's marked as nullable in the spec. I don't think we should use it in all cases.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For example, at https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/go-experimental/api.mustache#L26 With "typeMapping", this will produce NullableTime. I propose that we merge this PR as is, and then work on enhancing/fixing the typeMapping in a separate PR.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I agree this PR is ok as-is, let's keep more changes for followup PRs.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I have added code comments to explain.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks. Can you approve the PR then? Approving in the text does not help much. BTW, for some reason I am unable to add you as a reviewer.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Approved. Sorry, I keep forgetting doing the official review. |
||
| } else { | ||
| param.dataType = "Nullable" + Character.toUpperCase(param.dataType.charAt(0)) | ||
| + param.dataType.substring(1); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // The superclass determines the list of required golang imports. The actual list of imports | ||
| // depends on which types are used, which is done in the code above. So super.postProcessModels | ||
| // must be invoked at the end of this method. | ||
| objs = super.postProcessModels(objs); | ||
| return objs; | ||
| } | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using NullableTime for the OAS "time" type instead of plain "string" type? At least specifically for go-experimental, because this would be a breaking change for "go". Golang has the time.Time.Truncate() function, which could help to perform arithmetic with dates.