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

Add parcelizeModels CLI option #1289

Merged
merged 4 commits into from
Nov 9, 2018
Merged

Add parcelizeModels CLI option #1289

merged 4 commits into from
Nov 9, 2018

Conversation

absimas
Copy link
Contributor

@absimas absimas commented Oct 22, 2018

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, 3.4.x, 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

With cli option parcelizeModels (false by default) allow the use of kotlin's parcelize feature which generates all boilerplate automatically. Currently, the user will require to manually set

androidExtensions {
    experimental = true
}

but it's a small price to pay IMO.

@jimschubert @dr4ke616

@wing328
Copy link
Member

wing328 commented Oct 23, 2018

@absimas Thanks for the PR. Java client generator already supports this option as follows:

    public static final String PARCELABLE_MODEL = "parcelableModel";
    ......
    cliOptions.add(CliOption.newBoolean(PARCELABLE_MODEL, "Whether to generate models for Android that implement Parcelable with the okhttp-gson library."));

Shall we name it consistently using parcelableModel instead?

Ref: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/JavaClientCodegen.java#L64

@absimas
Copy link
Contributor Author

absimas commented Oct 23, 2018

I would disagree because parcelize is just an annotation that internally generates parcelable implementation for models.
parcelableModel generates the implementation itself which is not the same.

@wing328
Copy link
Member

wing328 commented Oct 23, 2018

Understood there's a slight difference. My intention is to have one single "Parcelable" option to make our life easier (we want to avoid adding too many options if possible)

Let's see if @jimschubert @dr4ke616 have opinions on this.

@absimas
Copy link
Contributor Author

absimas commented Oct 23, 2018

I understand the need but I think it's important to make the distinction here.
Since they're different there may be a need for parcelableModel in kotlin too.

@wing328 wing328 modified the milestones: 3.3.2, 3.3.3 Oct 31, 2018
@wing328
Copy link
Member

wing328 commented Nov 4, 2018

@absimas I ran some tests and got errors doing gradle test using the option --additional-properties parcelizeModels=true:

➜  kotlin_client gradle build

> Task :compileKotlin
Using kotlin incremental compilation

e: /private/tmp/kotlin_client/src/main/kotlin/org/openapitools/client/models/Pet.kt: (15, 8): Unresolved reference: android
e: /private/tmp/kotlin_client/src/main/kotlin/org/openapitools/client/models/Pet.kt: (16, 8): Unresolved reference: kotlinx
e: /private/tmp/kotlin_client/src/main/kotlin/org/openapitools/client/models/Pet.kt: (24, 2): Unresolved reference: Parcelize
e: /private/tmp/kotlin_client/src/main/kotlin/org/openapitools/client/models/Pet.kt: (29, 5): Unresolved reference: Parcelable

Spec: https://github.com/OAI/OpenAPI-Specification/blob/master/examples/v2.0/yaml/petstore-minimal.yaml

Do I need to update build.gradle to make it work?

@absimas
Copy link
Contributor Author

absimas commented Nov 4, 2018

@wing328 as I've said in the PRs description, you need to allow experimental features by adding the following lines to your build.gradle file:

androidExtensions {
    experimental = true
}

@wing328
Copy link
Member

wing328 commented Nov 4, 2018

@absimas actually I did try that (and other "solutions" I found in SO) earlier but no luck:

➜  kotlin_client3 gradle build

FAILURE: Build failed with an exception.

* Where:
Build file '/private/tmp/kotlin_client3/build.gradle' line: 22

* What went wrong:
A problem occurred evaluating root project 'kotlin-client'.
> Could not find method androidExtensions() for arguments [build_et5ydjjkf5wdkoo2a980cxo4s$_run_closure2@42fc727a] on root project 'kotlin-client' of type org.gradle.api.Project.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

Deprecated Gradle features were used in this build, making it incompatible with Gradle 5.0.
See https://docs.gradle.org/4.8/userguide/command_line_interface.html#sec:command_line_warnings

BUILD FAILED in 1s

Do I need to use a newer version of Gradle in order to recognize androidExtensions()?

I'm using Gradle 4.8.

@absimas
Copy link
Contributor Author

absimas commented Nov 4, 2018

For future reference: the problem is that the kotlin client doesn't depend on the android's framework which provides the base Parcelable that's required by the kotlin-android-extensions Parcelize.

@wing328
Copy link
Member

wing328 commented Nov 5, 2018

@absimas when you've time, please have a look at the compile issue reported by the CI below:

$ mvn --quiet clean install
[ERROR] COMPILATION ERROR : 
[ERROR] /home/travis/build/OpenAPITools/openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractKotlinCodegen.java:[364,14] ';' expected
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.5.1:compile (default-compile) on project openapi-generator: Compilation failure
[ERROR] /home/travis/build/OpenAPITools/openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractKotlinCodegen.java:[364,14] ';' expected
[ERROR] 

@wing328
Copy link
Member

wing328 commented Nov 6, 2018

@absimas can you please also run ./bin/utils/export_docs_generators.sh to ensure the documentation is up-to-date?

@wing328
Copy link
Member

wing328 commented Nov 6, 2018

If no further feedback/question on this PR, I'll merge it on coming Friday.

@wing328
Copy link
Member

wing328 commented Nov 9, 2018

The CircleCI failure is due to outdated generator doc and I'll update after merging this PR into master.

@wing328 wing328 merged commit f802e63 into OpenAPITools:master Nov 9, 2018
@wing328 wing328 changed the title Add parcelizeModels cli option Add parcelizeModels CLI option Nov 9, 2018
@wing328
Copy link
Member

wing328 commented Nov 15, 2018

@absimas thanks for the PR, which has been included in the v3.3.3 release: https://twitter.com/oas_generator/status/1062929948191510528

@absimas absimas deleted the parcelize-kotlin-models branch November 16, 2018 14:43
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* Add parcelizeModels cli option

* Add info log to clarify the parcelization requirements.

* Update docs
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.

None yet

2 participants