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.

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

[R client] Use %f instead of %d in sprintf format for numerics #3170

Closed
wants to merge 5 commits into from
Closed

[R client] Use %f instead of %d in sprintf format for numerics #3170

wants to merge 5 commits into from

Conversation

bmordue
Copy link
Contributor

@bmordue bmordue commented Jun 18, 2019

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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.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

This is a small fix for JSON serialization using sprintf of 'numeric' types in R that is already in swagger-codegen:
Relevant issue: swagger-api/swagger-codegen#8531

cc @andypetrella

Note: running the sample generation script ./bin/openapi3/r-petstore.sh has added new files.

@bmordue
Copy link
Contributor Author

bmordue commented Jun 19, 2019

Hmm. This PR passed 3/4 checks: Shippable failed with some kind of build error unrelated to this change.
I force-pushed the same file changes, but with an edited commit message, just to retrigger the build checks. (It looks like I can't manually retrigger a failed job on Shippable or CircleCI without repo write access).
Second time, 3/4 checks passed: CircleCI failed this time, again I believe unrelated to my change.

So all 4 checks have passed for this PR, but not at the same time.

@bmordue
Copy link
Contributor Author

bmordue commented Jun 21, 2019

@wing328 any thoughts about the failing build (timed out)?

@wing328
Copy link
Member

wing328 commented Jun 21, 2019

@bmordue there's a merged PR improving R's model: #3099

Please check it out.

             {{#isNumeric}}[%d]{{/isNumeric}}{{^isNumeric}}[%s]{{/isNumeric}}{{#hasMore}},{{/hasMore}}

I think we need to keep %d for integer and long using for example {{#isInteger}} ... {{/isInteger}}

@bmordue
Copy link
Contributor Author

bmordue commented Jun 21, 2019

@wing328 That's great, thanks for pointing that out.

@bmordue bmordue closed this Jun 21, 2019
@bmordue bmordue deleted the r-numeric-format branch June 21, 2019 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants