-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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] improve binary/text response handling #20131
[R] improve binary/text response handling #20131
Conversation
modules/openapi-generator/src/main/resources/r/api_exception.mustache
Outdated
Show resolved
Hide resolved
@@ -45,14 +45,11 @@ ApiResponse <- R6::R6Class( | |||
#' | |||
#' @param from_encoding The encoding of the raw response. | |||
#' @param to_encoding The target encoding of the return value. | |||
response_as_text = function(from_encoding = "", to_encoding = "UTF-8") { | |||
ResponseAsText = function(from_encoding = "", to_encoding = "UTF-8") { |
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.
shall we keep the snake case convention for the time being?
changing it results in breaking changes (without fallback) which is not something we want to do in the next minor release v7.11.0 (FYI we allow breaking changes with fallback in minor release)
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.
to use PascalCase, I can think of a solution using lambda but I prefer we deal with it in another PR instead.
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.
Sure, I'll revert to snake case. I switched to pearl based on the contributing guide, but I appreciate avoiding breaking changes.
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.
done
return(text) | ||
} | ||
return(self$api_client$deserialize(text, return_type, loadNamespace("{{packageName}}"))) | ||
} |
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.
what about adding these functions to api_client (which contains other utility functions)?
otherwise we will be repeating the same set of functions (private) in every auto-generated API files
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.
Done. Note that the code will be appear once in generated clients, but will need to be maintained in 2 places:
modules/openapi-generator/src/main/resources/r/api_client.mustache
modules/openapi-generator/src/main/resources/r/libraries/httr2/api_client.mustache
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.
thanks. we can use partial mustache files but that can be done in a separate PR instead.
@mattpollock thanks for the PR. I left some feedback. Definitely want to get this merged to improve the R client generator. Feel free to PM me via Slack to further discuss this PR or if you need any help: https://join.slack.com/t/openapi-generator/shared_invite/zt-2uoef5v0g-XGwo8~2oJ3EoziDSO1CmdQ |
if there's no further feedback from anyone. we will merge this PR over the weekend. thanks again for the contribution. |
@Ramanth, @saigiridhar21, @wing328
This PR fixes a bug when handling an endpoint that returns binary data (e.g.
application/gzip
). It is really a follow-up to #17626.PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master
(upcoming7.x.0
minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)