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] Enum R6Class Support, closes #3367 #5728

Merged
merged 40 commits into from
Jul 1, 2020
Merged

[R] Enum R6Class Support, closes #3367 #5728

merged 40 commits into from
Jul 1, 2020

Conversation

LiNk-NY
Copy link
Contributor

@LiNk-NY LiNk-NY commented Mar 27, 2020

Hi all,

We've (@mtmorgan, @dvantwisk) provided an R6 class based on guidelines in issue #3367.

This PR includes:

  • modelEnum.mustache
  • changes to methods within R6Class: fromJSONString, fromJSON, etc.
  • removal of stray commas: function(, ...) to function(...)
  • use of httr::content(resp) instead of apiResponse$content
  • updates to the petstore API by running ./bin/r-petstore.sh

Diff of the changes: master...LiNk-NY:enum

Example implementation from the HCAMatrix Service:

    v1_ComparisonFilterOperator:
      description: 'Comparison operators allowed in matrix filters.'
      type: string
      enum:
        - '='
        - '!='
        - '>'
        - '<'
        - '>='
        - '<='
        - 'in'
# HCA Matrix Service
#
# Human Cell Atlas Matrix Service API
#
# The version of the OpenAPI document: 1.0.0
# 
# Generated by: https://openapi-generator.tech

#' @docType class
#' @title V1ComparisonFilterOperator
#'
#' @description V1ComparisonFilterOperator Class
#'
#' @format An \code{R6Class} generator object
#'
#' @importFrom R6 R6Class
#' @importFrom jsonlite fromJSON toJSON
# add to utils.R
.parse_v1_ComparisonFilterOperator <- function(vals) {
    res <- gsub("^\\[|\\]$", "",
        "[=, !=, >, <, >=, <=, in]"
    )
    unlist(strsplit(res, ", "))
}

#' @export
V1ComparisonFilterOperator <- R6::R6Class(
    "V1ComparisonFilterOperator",
    public = list(
        initialize = function(...) {
            local.optional.var <- list(...)
            val <- unlist(local.optional.var)
            enumvec <- .parse_v1_ComparisonFilterOperator()

            stopifnot(length(val) == 1L)

            if (!val %in% enumvec)
                stop("Use one of the valid values: ",
                    paste0(enumvec, collapse = ", "))
            private$value <- val
        },
        toJSON = function() {
            jsonlite::toJSON(private$value, auto_unbox = TRUE)
        },
        fromJSON = function(V1ComparisonFilterOperatorJson) {
            private$value <- jsonlite::fromJSON(V1ComparisonFilterOperatorJson,
                simplifyVector = FALSE)
            self
        },
        toJSONString = function() {
            as.character(jsonlite::toJSON(private$value,
                auto_unbox = TRUE))
        },
        fromJSONString = function(V1ComparisonFilterOperatorJson) {
            private$value <- jsonlite::fromJSON(V1ComparisonFilterOperatorJson,
                simplifyVector = FALSE)
            self
        }
    ),
    private = list(
        value = NULL
    )
)

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

Best regards,
Marcel

cc @Leo-Rutschmann @wing328 @ramnov

To close #3367

LiNk-NY and others added 30 commits November 18, 2019 23:56
@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented Apr 20, 2020

Hi William, @wing328
Any updates on the PR? Thank you.

@wing328
Copy link
Member

wing328 commented Apr 29, 2020

Sorry too busy.

@Ramanth I wonder if you can review this PR when you've time. Thank you.

@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented May 5, 2020

Thanks Will.
Any updates @Ramanth ?

@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented May 11, 2020

Any updates on the PR? @wing328 @Ramanth

@wing328
Copy link
Member

wing328 commented May 12, 2020

UPDATE: I've merged the latest master of the official repo into your branch. Let's see if that fixes the CircleCI, Travis CI build failures.

@wing328
Copy link
Member

wing328 commented May 12, 2020

CircleCI reports the following errors:

  All declared Imports should be used.
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... OK
* checking for missing documentation entries ... WARNING
Undocumented code objects:
  ‘ApiClient’ ‘ApiResponse’ ‘Category’ ‘ModelApiResponse’ ‘Order’ ‘Pet’
  ‘PetApi’ ‘StoreApi’ ‘Tag’ ‘User’ ‘UserApi’
All user-level objects in a package should have documentation entries.
See chapter ‘Writing R documentation files’ in the ‘Writing R
Extensions’ manual.
* checking examples ... NONE
* checking for unstated dependencies in ‘tests’ ... OK
* checking tests ...
  Running ‘testthat.R’
 ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  ── 2. Error: GetPetById (@test_petstore.R#45)  ─────────────────────────────────
  cannot unclass an environment
  Backtrace:
   1. testthat::expect_equal(...)
   3. testthat:::compare.default(act$val, exp$val, ...)
   5. base::all.equal.list(x, y, ...)
   7. base::all.equal.list(...)
  
  ══ testthat results  ═══════════════════════════════════════════════════════════
  [ OK: 6 | SKIPPED: 48 | WARNINGS: 0 | FAILED: 2 ]
  1. Failure: AddPet (@test_petstore.R#17) 
  2. Error: GetPetById (@test_petstore.R#45) 
  
  Error: testthat unit tests failed
  Execution halted
* DONE

Status: 1 ERROR, 2 WARNINGs, 1 NOTE
See
  ‘/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/R/petstore.Rcheck/00check.log’
for details.

[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (nose-test) on project RPetstoreClientTests: Command execution failed. Process exited with an error: 1 (Exit value: 1) -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
[ERROR] 
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR]   mvn <goals> -rf :RPetstoreClientTests

I wonder if you can take a look when you've time.

@wing328
Copy link
Member

wing328 commented May 12, 2020

For the Travis CI failure, I really have no clue as I don't see any changes to the python samples:

=============== 11 passed, 9 skipped, 1 warnings in 6.70 seconds ===============
___________________________________ summary ____________________________________
  py3: commands succeeded
  congratulations :)
petstore_api/:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: 'petstore_api/'
make: *** [test-all] Error 1
Makefile:20: recipe for target 'test-all' failed
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (test) on project PythonFlaskConnexionTests: Command execution failed. Process exited with an error: 2 (Exit value: 2) -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 

- export tag does not coincide with original model function
- move helper down so it does
@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented May 12, 2020

Hi Will, @wing328

Thank you for taking a look at it.

The enum functionality did not introduce any additional tests. I can look into why
it's breaking.

As a side note, based on what I have seen in the original implementation, I wouldn't
expect R CMD check to pass on the package. There are numerous issues with
the generation of the R client such as with the documentation and NAMESPACE.
I am working on these issues separately.
-Marcel

@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented May 12, 2020

Hi Will, @wing328

I noticed that when running ./bin/r-petstore.sh, the tests in the tests directory for the R package do not get overwritten. I deleted the old test files and re-ran the ./bin/r-petstore.sh script.

Should overwriting be the default when generating the R client SDK? Perhaps, we could
make that change in the script file. Or at least make it an option.

Thanks,
Marcel

@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented May 12, 2020

Something like:

diff --git a/bin/r-petstore.sh b/bin/r-petstore.sh
index 4decf28f72..abb7095109 100755
--- a/bin/r-petstore.sh
+++ b/bin/r-petstore.sh
@@ -25,8 +25,14 @@ then
   mvn -B clean package
 fi
 
+PET_PKG="samples/client/petstore/R"
+
+if [ -d "${PET_PKG}" ]; then
+    rm -rf ${PET_PKG}
+fi
+
 # if you've executed sbt assembly previously it will use that instead.
 export JAVA_OPTS="${JAVA_OPTS} -Xmx1024M -DloggerPath=conf/log4j.properties"
-ags="generate -t modules/openapi-generator/src/main/resources/r -i modules/openapi-generator/src/test/resources/2_0/petstore.yaml -g r -o samples/client/petstore/R --additional-properties packageName=petstore $@"
+ags="generate -t modules/openapi-generator/src/main/resources/r -i modules/openapi-generator/src/test/resources/2_0/petstore.yaml -g r -o ${PET_PKG} --additional-properties packageName=petstore $@"
 
 java $JAVA_OPTS -jar $executable $ags

@wing328
Copy link
Member

wing328 commented May 13, 2020

the tests in the tests directory for the R package do not get overwritten

That's by design as users may have manually written some tests.

Can you please restore those tests and update the tests to make it work with enum enhancement in this PR instead?

@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented May 19, 2020

Hi William, @wing328, Ramanth @Ramanth
I've made changes and updated the unit tests in the package. It's ready for another review.
Thanks.
-Marcel

Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

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

LGTM but I've not tested it locally.

@wing328
Copy link
Member

wing328 commented Jul 1, 2020

@LiNk-NY thanks for the contribution, which has been merged into master.

@wing328 wing328 added this to the 5.0.0 milestone Jul 3, 2020
jimschubert added a commit that referenced this pull request Jul 3, 2020
* master: (142 commits)
  update python samples
  clarify direction of py client side validation flag (#6850)
  fix erronous cmd arg example for docker in readme (#6846)
  [BUG] [JAVA] Fix multiple files upload (#4803) (#6808)
  [kotlin][client] fix retrofit dependencies (#6836)
  [PowerShell] add more fields to be customized (#6835)
  [Java][WebClient]remove the dead code from java ApiClient.mustache (#6556)
  [PHP] Better handling of invalid data (array) (#6760)
  Make ApiClient in retrofit2 be able to use own OkHttpClient (#6699)
  mark python2 support in flask as deprecated (#6653)
  update samples
  [Java][jersey2] Add a getter for the User-Agent header value (#6831)
  Provides a default nil value for optional init parameters (#6827)
  [Java] Deprecate feignVersion option (#6824)
  [R] Enum R6Class Support, closes #3367 (#5728)
  [Rust][Client] Unify sync/async client structure (#6753)
  [php-ze-ph] Set required PHP version to ^7.2 (#6763)
  [Java][client][native][Gradle] Add missing jackson-databind-nullable (#6802)
  Improve sttpOpenApiClient generator (#6684)
  Update docker-tag-latest-release.yml
  ...
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.

[BUG][R] Generating R enums results in empty class with no values
3 participants