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] Add new R client generator #6351

Merged
merged 9 commits into from
Sep 3, 2017
Merged

[R] Add new R client generator #6351

merged 9 commits into from
Sep 3, 2017

Conversation

wing328
Copy link
Contributor

@wing328 wing328 commented Aug 22, 2017

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 for non-breaking changes and 3.0.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Add R client generator based on R Petstore client provided by @ramnov

Still WIP

if (httr::status_code(resp) >= 200 && httr::status_code(resp) <= 299) {
parsed <- jsonlite::fromJSON(httr::content(resp, "text", encoding = "UTF-8"),
simplifyVector = FALSE)
result <- Pet$fromJSON(parsed)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ramnov is there any way we can provide a fromJSON method to construct the object based on a JSON string?

For other languages, there are libraries for (de)serialization between JSON and object. I wonder if R has similar package.

Copy link
Contributor

@ramnov ramnov Aug 27, 2017

Choose a reason for hiding this comment

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

There is no package in R to deserialize/serialize between R6 object and JSON. fromJSON() and toJSON() methods have to be implemented by ourselves in R6 Classes. I believe Pet.R already has a toJSON() method. Can you add this fromJSON() method as well to Pet Class in Pet.R :

fromJSON = function(petJson) {
petObject <- jsonlite::fromJSON(petJson)
self$id <- petObject$id
self$category <- Element$new(petObject$category$id, petObject$category$name)
self$name <- petObject$name
self$photoUrls <- petObject$photoUrls
self$tags <- lapply(petObject$tags, function(x) Element$new(x$id, x$name))
self$status <- petObject$status
}

And then this function can be used in PetApi.R like this :

result <- Pet$new()$fromJSON(httr::content(resp, "text", encoding = "UTF-8"),
simplifyVector = FALSE)

resp <- httr::GET(paste0(self$url),
httr::add_headers("User-Agent" = self$userAgent, "content-type" = "application/xml")
)
# TODO add support for query parameters "status"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ramnov I'm not sure how query string can be added to httpr? Is there something like httr::add_queries? I couldn't find one in https://www.rdocumentation.org/packages/httr/versions/1.2.1

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a query parameter in GET function.
Example : GET("http://httpbin.org/cookies/set", query = list(a = 1, b = 2))
will translate to http://httpbin.org/cookies/set?a=1&b=1
More Examples here : https://stackoverflow.com/questions/32742535/difference-between-query-and-body-in-httrpost

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ramnov OK. Will give that a try.

basePath = NULL,
scheme = NULL,
url = NULL,
initialize = function(host, basePath, scheme){
Copy link
Contributor Author

@wing328 wing328 Aug 23, 2017

Choose a reason for hiding this comment

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

@ramnov Can host, basePath, scheme be optional and we'll provide the default value based on the spec? For example, the default value for basePath is v2: https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/test/resources/2_0/petstore.yaml#L13

Copy link
Contributor

Choose a reason for hiding this comment

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

default values can be set during definition itself. To allow for optional arguments in initialize function, we just need to check whether the argument is "missing". Here is an example of setting default values and optional arguments in PetApi class :

PetApi <- R6::R6Class(
'PetApi',
public = list(
userAgent = "Swagger-Codegen/1.0.0/r",
host = "petstore.swagger.io",
basePath = "v2",
scheme = "http",
url = NULL,
initialize = function(host, basePath, scheme){
if (!missing(host)) {
stopifnot(is.character(host), length(host) == 1)
self$host <- host
}
if (!missing(basePath)) {
stopifnot(is.character(basePath), length(basePath) == 1)
self$basePath <- basePath
}
if (!missing(scheme)) {
stopifnot(is.character(scheme), length(scheme) == 1)
self$scheme <- scheme
}
self$url <- sprintf("%s://%s/%s/pet/", scheme, host, basePath)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. Will incorporate these changes later today.

parsed <- jsonlite::fromJSON(httr::content(resp, "text", encoding = "UTF-8"),
simplifyVector = FALSE)
result <- {{returnType}}$fromJSON(parsed)
result <- {{returnType}}$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

{{returnType}}$new()$fromJSON

{{/isPrimitiveType}}
{{^isPrimitiveType}}
{{#isListContainer}}
self${{name}} <- lapply({{classname}}Object${{name}}, function(x) {{datatype}}$new()$fromJSON(x)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, you are passing a data frame to fromJSON method. You should pass a JSON string like this :

self${{name}} <- lapply({{classname}}Object${{name}}, function(x) {{datatype}}$new()$fromJSON(jsonlite::toJSON(x))

self$id <- OrderObject$id
self$pet_id <- OrderObject$pet_id
self$quantity <- OrderObject$quantity
self$ship_date <- Date$new()$fromJSON(OrderObject$ship_date)
Copy link
Contributor

Choose a reason for hiding this comment

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

Again you need to pass JSON to fromJSON rather than data frame :

Date$new()$fromJSON(jsonlite::toJSON(OrderObject$ship_date))

fromJSON = function(PetJson) {
PetObject <- jsonlite::fromJSON(PetJson)
self$id <- PetObject$id
self$category <- Category$new()$fromJSON(PetObject$category)
Copy link
Contributor

Choose a reason for hiding this comment

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

Category$new()$fromJSON(jsonlite::toJSON(PetObject$category))

self$category <- Category$new()$fromJSON(PetObject$category)
self$name <- PetObject$name
self$photo_urls <- PetObject$photo_urls
self$tags <- lapply(PetObject$tags, function(x) Tag$new()$fromJSON(x)
Copy link
Contributor

Choose a reason for hiding this comment

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

self$tags <- lapply(PetObject$tags, function(x) Tag$new()$fromJSON(jsonlite::toJSON(x))

parsed <- jsonlite::fromJSON(httr::content(resp, "text", encoding = "UTF-8"),
simplifyVector = FALSE)
result <- Pet$fromJSON(parsed)
result <- Pet$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

result <- Pet$new()$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)

parsed <- jsonlite::fromJSON(httr::content(resp, "text", encoding = "UTF-8"),
simplifyVector = FALSE)
result <- Pet$fromJSON(parsed)
result <- Pet$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

result <- Pet$new()$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)

parsed <- jsonlite::fromJSON(httr::content(resp, "text", encoding = "UTF-8"),
simplifyVector = FALSE)
result <- Pet$fromJSON(parsed)
result <- Pet$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

result <- Pet$new()$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)

parsed <- jsonlite::fromJSON(httr::content(resp, "text", encoding = "UTF-8"),
simplifyVector = FALSE)
result <- ApiResponse$fromJSON(parsed)
result <- ApiResponse$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

result <- ApiResponse$new()$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)

parsed <- jsonlite::fromJSON(httr::content(resp, "text", encoding = "UTF-8"),
simplifyVector = FALSE)
result <- Integer$fromJSON(parsed)
result <- Integer$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

result <- Integer$new()$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)

parsed <- jsonlite::fromJSON(httr::content(resp, "text", encoding = "UTF-8"),
simplifyVector = FALSE)
result <- Order$fromJSON(parsed)
result <- Order$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

result <- Order$new()$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)

parsed <- jsonlite::fromJSON(httr::content(resp, "text", encoding = "UTF-8"),
simplifyVector = FALSE)
result <- Order$fromJSON(parsed)
result <- Order$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

result <- Order$new()$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)

parsed <- jsonlite::fromJSON(httr::content(resp, "text", encoding = "UTF-8"),
simplifyVector = FALSE)
result <- User$fromJSON(parsed)
result <- User$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

result <- User$new()$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)

parsed <- jsonlite::fromJSON(httr::content(resp, "text", encoding = "UTF-8"),
simplifyVector = FALSE)
result <- Character$fromJSON(parsed)
result <- Character$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

result <- Character$new()$fromJSON(httr::content(resp, "text", encoding = "UTF-8"), simplifyVector = FALSE)

self${{name}} <- lapply({{classname}}Object${{name}}, function(x) {{datatype}}$new()$fromJSON(x)
{{/isListContainer}}
{{^isListContainer}}
self${{name}} <- {{datatype}}$new()$fromJSON({{classname}}Object${{name}})
Copy link
Contributor

Choose a reason for hiding this comment

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

self${{name}} <- {{datatype}}$new()$fromJSON(jsonlite::toJSON({{classname}}Object${{name}}))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will also change line 88 from

      self${{name}} <- lapply({{classname}}Object${{name}}, function(x) {{datatype}}$new()$fromJSON(x)

to

      self${{name}} <- lapply({{classname}}Object${{name}}, function(x) {{datatype}}$new()$fromJSON(jsonlite::toJSON(x))

"{{baseName}}" = {{paramName}}{{#hasMore}},{{/hasMore}}
{{/isFile}}
{{#isFile}}
"{{baseName}}" = upload_file({{paramName}}){{#hasMore}},{{/hasMore}}
Copy link
Contributor

Choose a reason for hiding this comment

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

httr::upload_file

@@ -134,7 +134,7 @@ PetApi <- R6::R6Class(
httr::add_headers("User-Agent" = self$userAgent, "accept" = "multipart/form-data", "content-type" = "application/json")
,body = list(
"additionalMetadata" = additional_metadata,
"file" = file #TODO support file upload
"file" = upload_file(file)
Copy link
Contributor

Choose a reason for hiding this comment

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

httr::upload_file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed via d76ab6d

@wing328 wing328 merged commit a4c0975 into master Sep 3, 2017
@wing328 wing328 deleted the r_generator branch September 11, 2017 09:39
karussell added a commit to graphhopper/directions-api-clients that referenced this pull request Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants