Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[C++][Restbed] Fix default values for Restbed Server generator #761

Merged
merged 2 commits into from
Sep 12, 2018
Merged

[C++][Restbed] Fix default values for Restbed Server generator #761

merged 2 commits into from
Sep 12, 2018

Conversation

stkrwork
Copy link
Contributor

@stkrwork stkrwork commented Aug 7, 2018 •
edited
Loading

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./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\.
  • Filed the PR against the correct branch: master, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Fix #760

@ravinikam @fvarose @etherealjoy @MartinDelille @wing328

@stkrwork stkrwork changed the title Start working on fixing default value in Restbed Server Api Template [C++][Restbed] Fix default values for Restbed Server generator Aug 7, 2018
@stkrwork
Copy link
Contributor Author

stkrwork commented Aug 7, 2018

Currently what seems to be not working is that if the variable is a string, the default value specified in the java file is not taken, but if it is a bool for example, it does work.

@etherealjoy
Copy link
Contributor

@stkrwork
I tried debugging the case for the string and strangely, toDefaultValue is called for string but the {{{DefaultValue}}} is having nothing.

@stkrwork stkrwork added the WIP Work in Progress label Aug 14, 2018
@stkrwork
Copy link
Contributor Author

So somewhere in the codegen base it doesnt get forwarded?

@wing328
Copy link
Member

wing328 commented Aug 14, 2018 •
edited
Loading

I just fixed the default value in C# (12.3F instead of 12.3). I can take a look at this tomorrow weekend.

@wing328
Copy link
Member

wing328 commented Aug 19, 2018 •
edited
Loading

UPDATE: I've pushed the fix (80b3eff). Please take a look.

I also did some tests to confirm the default value is now working fine with the cpp restbed server generator.

Since the fix is in DefaultCodegen, I've copied the core team as well.

cc @OpenAPITools/generator-core-team

(I'm verifying the impact to other generators)

etherealjoy
etherealjoy reviewed Aug 19, 2018
View reviewed changes
...napi-generator/src/main/java/org/openapitools/codegen/languages/CppRestbedServerCodegen.java
@@ -287,25 +291,63 @@ public String getTypeDeclaration(Schema p) {
@Override
public String toDefaultValue(Schema p) {
if (ModelUtils.isStringSchema(p)) {
return "\"\"";
if (p.getDefault() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we need to do this for other generators as well right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most other generators are following this approach but I'll double check to make sure.

I'm thinking about another PR to clean up the default, example value as the fix in DefaultCodegen in this PR seems to be partial (which means other generators are showing different default, example values after the change)

@etherealjoy
Copy link
Contributor

Looks good though.

@stkrwork
Copy link
Contributor Author

looks good to me

@stkrwork stkrwork removed the WIP Work in Progress label Aug 19, 2018
@stkrwork
Copy link
Contributor Author

can you check if your problem is fixed @vbs96?

ackintosh
ackintosh approved these changes Sep 2, 2018
View reviewed changes
Copy link
Contributor

@ackintosh ackintosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in DefaultCodegen looks good to me.

@stkrwork stkrwork force-pushed the restbed_fix_default_value branch 3 times, most recently from f454625 to 7b42b16 Compare September 10, 2018 21:49
@stkrwork stkrwork merged commit f29ba97 into OpenAPITools:master Sep 12, 2018
@wing328
Copy link
Member

wing328 commented Sep 12, 2018 •
edited
Loading

@stkrwork as discussed, we'll need to revert this for the time being as other generators (e.g. Java) are impacted by this. (Done via #1027)

wing328 added a commit that referenced this pull request Sep 12, 2018
@wing328
Revert "[C++][Restbed] Fix default values for Restbed Server generator (
663ad27 
#761)"

This reverts commit f29ba97.
@wing328 wing328 mentioned this pull request Sep 12, 2018
Merged
wing328 added a commit that referenced this pull request Sep 13, 2018
@wing328
Revert "[C++][Restbed] Fix default values for Restbed Server generato…
096ac56 
…r" (#1027)

* Revert "[gradle plugin] Support Gradle 4.10 (#1011)"

This reverts commit 131cf94.

* Revert "[C++][Restbed] Fix default values for Restbed Server generator (#761)"

This reverts commit f29ba97.
This was referenced Sep 13, 2018
Closed
Merged
jaumard pushed a commit to jaumard/openapi-generator that referenced this pull request Sep 21, 2018
@stkrwork @jaumard
[C++][Restbed] Fix default values for Restbed Server generator (OpenA…
afa7ebc 
…PITools#761)

* Start working on fixing default value in Restbed Server Api Template

* fix default value in DefaultCodegen
jaumard pushed a commit to jaumard/openapi-generator that referenced this pull request Sep 21, 2018
@wing328 @jaumard
Revert "[C++][Restbed] Fix default values for Restbed Server generato…
6de8404 
…r" (OpenAPITools#1027)

* Revert "[gradle plugin] Support Gradle 4.10 (OpenAPITools#1011)"

This reverts commit 131cf94.

* Revert "[C++][Restbed] Fix default values for Restbed Server generator (OpenAPITools#761)"

This reverts commit f29ba97.
@wing328 wing328 added this to the 3.3.0 milestone Oct 1, 2018
@wing328
Copy link
Member

wing328 commented Oct 2, 2018 •
edited
Loading

@stkrwork thanks again for the PR, which is included in the v3.3.0 minor release: https://twitter.com/oas_generator/status/1046941449609068544

Sorry, this change is not included in the 3.3.0 release as it has been rolled back.

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
@stkrwork
[C++][Restbed] Fix default values for Restbed Server generator (OpenA…
88b2ce1 
…PITools#761)

* Start working on fixing default value in Restbed Server Api Template

* fix default value in DefaultCodegen
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
@wing328
Revert "[C++][Restbed] Fix default values for Restbed Server generato…
e3aeab1 
…r" (OpenAPITools#1027)

* Revert "[gradle plugin] Support Gradle 4.10 (OpenAPITools#1011)"

This reverts commit 131cf94.

* Revert "[C++][Restbed] Fix default values for Restbed Server generator (OpenAPITools#761)"

This reverts commit f29ba97.
This was referenced Mar 11, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wing328 wing328 wing328 left review comments

@etherealjoy etherealjoy etherealjoy approved these changes

@MartinDelille MartinDelille MartinDelille approved these changes

@ackintosh ackintosh ackintosh approved these changes

Assignees
No one assigned
Labels
Issue: Bug Server: C++
Projects
None yet
Milestone
3.3.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@stkrwork @etherealjoy @wing328 @MartinDelille @ackintosh