-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[cpp-restsdk] Fix handling of UUID types #554
[cpp-restsdk] Fix handling of UUID types #554
Conversation
@Danielku15 I believe you're the required reviewer for this PR as well |
@Peaches491 thanks for the PR. cc'ing the technical committee for review: @ravinikam (2017/07) @stkrwork (2017/07) @fvarose (2017/11) @etherealjoy (2018/02) @MartinDelille (2018/03) |
@@ -300,7 +300,7 @@ public String getTypeDeclaration(Schema p) { | |||
return getSchemaType(p) + "<utility::string_t, " + getTypeDeclaration(inner) + ">"; | |||
} else if (ModelUtils.isStringSchema(p) | |||
|| ModelUtils.isDateSchema(p) || ModelUtils.isDateTimeSchema(p) | |||
|| ModelUtils.isFileSchema(p) | |||
|| ModelUtils.isFileSchema(p) || ModelUtils.isUUIDSchema(p) |
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.
@Peaches491 thanks again for the PR. Should the isStringSchema
check at line 301 already cover that (type: string)?
public static boolean isStringSchema(Schema schema) {
if (schema instanceof StringSchema || SchemaTypeUtil.STRING_TYPE.equals(schema.getType())) {
return true;
}
return false;
}
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.
I don't believe so, since adding this caused the generated code to be different. Specifically, string parameters with format: "uuid"
were no longer used a std::shared_ptr<utilities::string_t>
and instead just used the expected utilities::string_t
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.
You're right. I did a test with latest master and got:
int64_t petId,
- boost::optional<utility::string_t> name,
+ boost::optional<std::shared_ptr<utility::string_t>> name,
boost::optional<utility::string_t> status
@Peaches491 thanks for the PR, which has been merged into master. |
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\
.master
,3.1.x
,4.0.x
. Default:master
.Description of the PR
Fixes handling of UUID types, preventing the generation of
std::shared_ptr<utility::string_t>
members in models.NOTE: This PR does not effect the petshop sample, as
petshop.json
does not make use offormat: "uuid"