Skip to content

[Java][Retrofit] Add RxJava2 support#4708

Merged
wing328 merged 2 commits intoswagger-api:masterfrom
locationlabs:feature/issue-4698_rxjava2-support
Feb 7, 2017
Merged

[Java][Retrofit] Add RxJava2 support#4708
wing328 merged 2 commits intoswagger-api:masterfrom
locationlabs:feature/issue-4698_rxjava2-support

Conversation

@ber4444
Copy link
Copy Markdown
Contributor

@ber4444 ber4444 commented Feb 3, 2017

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch 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)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

#4698

wing328
wing328 reviewed Feb 5, 2017
View reviewed changes
modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/JavaClientCodegen.java Outdated

cliOptions.add(CliOption.newBoolean(USE_RX_JAVA, "Whether to use the RxJava adapter with the retrofit2 library."));
cliOptions.add(CliOption.newBoolean(USE_RX_JAVA2, "Whether to use the RxJava2 adapter with the retrofit2 library."));
cliOptions.add(CliOption.newBoolean(DO_NOT_USE_RX, "If you do not use RxJava[2], Retrofit's default Call type will be generated instead.")); // setting this to false will have undefined results
Copy link
Copy Markdown
Contributor

@wing328 wing328 Feb 5, 2017

Choose a reason for hiding this comment

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

@ber4444 I suggest we don't expose the option DO_NOT_USE_RX as it should be not set by the users if I understand correctly.

wing328
wing328 reviewed Feb 5, 2017
View reviewed changes
modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/JavaClientCodegen.java
}
if (additionalProperties.containsKey(USE_RX_JAVA2)) {
this.setUseRxJava2(Boolean.valueOf(additionalProperties.get(USE_RX_JAVA2).toString()));
}
Copy link
Copy Markdown
Contributor

@wing328 wing328 Feb 5, 2017

Choose a reason for hiding this comment

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

If both USE_RX_JAVA and USE_RX_JAVA2 are incorrectly set to true, maybe it would be a good idea to log a warning/error and only set one of them to true only.

@wing328 wing328 added this to the v2.2.2 milestone Feb 5, 2017
@wing328 wing328 merged commit d12f428 into swagger-api:master Feb 7, 2017
@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Feb 7, 2017

@ber4444 I've done some tests and the results look good.

Thanks for your contribution.

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Feb 7, 2017

Actually I discovered some issues. Let me roll back first before working on a fix.

@wing328 wing328 mentioned this pull request Feb 7, 2017
Merged
wing328
wing328 reviewed Feb 7, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

Please review and let me know if you need any help fixing the issue.

modules/swagger-codegen/src/main/resources/Java/libraries/retrofit2/pom.mustache
</dependency>
<dependency>
<groupId>com.jakewharton.retrofit</groupId>
<artifactId>com.jakewharton.retrofit</artifactId>
Copy link
Copy Markdown
Contributor

@wing328 wing328 Feb 7, 2017

Choose a reason for hiding this comment

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

I think this should be <artifactId>retrofit2-rxjava2-adapter</artifactId> instead.

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/JavaClientCodegen.java
}
if (additionalProperties.containsKey(DO_NOT_USE_RX)) {
this.setDoNotUseRx(Boolean.valueOf(additionalProperties.get(DO_NOT_USE_RX).toString()));
}
Copy link
Copy Markdown
Contributor

@wing328 wing328 Feb 7, 2017

Choose a reason for hiding this comment

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

I think these 3 lines can be removed as we no longer allow setting DO_NOT_USE_RX via option.

And after processing all the RX-related option, we will need to update "doNotUseRx" in additionalProperties so that it has a proper value (true or false)

I tested without any RX-related option and got the following compilation errors:

[ERROR] /private/var/tmp/java/useRxJava20/src/main/java/io/swagger/client/api/PetApi.java:[127,3] cannot find symbol
[ERROR] symbol:   class Observable
[ERROR] location: interface io.swagger.client.api.PetApi

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Feb 7, 2017

I've discovered 2 minor issues. Please work on those and submit a new PR for the enhancement.

@wing328 wing328 mentioned this pull request Feb 7, 2017
Closed
Stwissel added a commit to Stwissel/swagger-codegen that referenced this pull request Feb 9, 2017
@Stwissel
Merge branch 'master' of github.com:swagger-api/swagger-codegen
0d045f5 
* 'master' of github.com:swagger-api/swagger-codegen: (40 commits)
  remove default temp folder during initalization (swagger-api#4749)
  [Java-retrofit] Fix for swagger-api#4750 String comparison with equals (swagger-api#4751)
  update java server stub samples with new uuid mapping
  update java petstore with new uuid mapping
  [Java-Feign] Fixed String comparison using equals instead of == operator (swagger-api#4740)
  add SPINEN
  update jaxrs spec petstore sample (mac)
  [Jaxrs-spec] fix usage of Jersey templates in shellscript (swagger-api#4722)
  [Bash] Bash generator improvements (swagger-api#4730)
  [Java][Issue swagger-api#1806] generate using java.util.UUID for UUIDs
  Revert "rx2 support" (swagger-api#4737)
  rx2 support (swagger-api#4708)
  add https and change order for HPE
  add Hewlett Packard Enterprise (hpe.com)
  Add "Simpfony" to list of companies using Swagger (swagger-api#4726)
  add https://www.slamby.com/
  [csharp] Fix enum default value (swagger-api#4681)
  fix issue swagger-api#4672 - XmlExampleGenerator does not properly handle properties of several numeric types (swagger-api#4673)
  [JAXRS-CXF] Issue 4569 - Re-added usage of contextPath in api.mustache (basePath) (swagger-api#4580)
  [Jaxrs-cxf-cdi] merge beanvalidation templates to single one swagger-api#4719 (swagger-api#4723)
  ...
Stwissel added a commit to Stwissel/swagger-codegen that referenced this pull request Feb 9, 2017
@Stwissel
Merge branch 'master' into feature/domino-server
d944e03 
* master: (40 commits)
  remove default temp folder during initalization (swagger-api#4749)
  [Java-retrofit] Fix for swagger-api#4750 String comparison with equals (swagger-api#4751)
  update java server stub samples with new uuid mapping
  update java petstore with new uuid mapping
  [Java-Feign] Fixed String comparison using equals instead of == operator (swagger-api#4740)
  add SPINEN
  update jaxrs spec petstore sample (mac)
  [Jaxrs-spec] fix usage of Jersey templates in shellscript (swagger-api#4722)
  [Bash] Bash generator improvements (swagger-api#4730)
  [Java][Issue swagger-api#1806] generate using java.util.UUID for UUIDs
  Revert "rx2 support" (swagger-api#4737)
  rx2 support (swagger-api#4708)
  add https and change order for HPE
  add Hewlett Packard Enterprise (hpe.com)
  Add "Simpfony" to list of companies using Swagger (swagger-api#4726)
  add https://www.slamby.com/
  [csharp] Fix enum default value (swagger-api#4681)
  fix issue swagger-api#4672 - XmlExampleGenerator does not properly handle properties of several numeric types (swagger-api#4673)
  [JAXRS-CXF] Issue 4569 - Re-added usage of contextPath in api.mustache (basePath) (swagger-api#4580)
  [Jaxrs-cxf-cdi] merge beanvalidation templates to single one swagger-api#4719 (swagger-api#4723)
  ...
@wing328 wing328 changed the title rx2 support [Java][Retrofit] Add RxJava2 support Feb 20, 2017
davidgri pushed a commit to davidgri/swagger-codegen that referenced this pull request May 11, 2017
@ber4444 @wing328
rx2 support (swagger-api#4708)
84ea2a6 
* rx2 support

* NO_NOT_USE_RX is for internal use only; plus sanity check if both v1 and v2 are specified
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@wing328 wing328 wing328 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.2.2

Development

Successfully merging this pull request may close these issues.

2 participants

@ber4444 @wing328