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

[Kotlin][retrofit2] replace okhttpclient with callfactory #9451

Conversation

shanselm-ergon
Copy link
Contributor

@shanselm-ergon shanselm-ergon commented May 11, 2021

[Kotlin][retrofit2] replace okhttpclient with callfactory

fixes [#9448]

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    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*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.1.x, 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jimschubert (2017/09) ❤️, @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10) @4brunu (2019/11) @yutaka0m (2020/03)

@shanselm-ergon
Copy link
Contributor Author

Can someone explain me, what the problem with the CI build is?

@4brunu
Copy link
Contributor

4brunu commented May 11, 2021

I think the problem is not related to this PR

[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (go-get) on project GoEchoServer: 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] 
[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 :GoEchoServer

@4brunu
Copy link
Contributor

4brunu commented May 18, 2021

@shanselm-ergon first of all, thanks for contributing to the kotlin client generator.
The problem I see with this, is that this will create a breaking change without fallback, this implies to targeting the branch 6.0.x.
Do you have any idea on how to make this changes backwards compatible?

@shanselm-ergon
Copy link
Contributor Author

The only way to keep this backwards compatible would be, to use the same parameter name okHttpClient for the Call.Factory. The rest would still be compatible, because every OkHttpClient is also a Call.Factory. Under the hood we anyway used the client only as factory, so there would not be a change for the users. The naming would just not be as accurate as it could be.

@4brunu
Copy link
Contributor

4brunu commented May 19, 2021

@shanselm-ergon Thanks a lot for your explanation.
Since OkHttpClient is also a Call.Factory, I think maybe we can keep this PR as is and consider it a breaking change with fallback because if the users of this library don't use named parameters, they don't need to change anything, and if they use, the only thing that they have to do is to change the name from okHttpClient to callFactory which is a compile time error, and the IDE helps with the migration, so it seems reasonable to me.
Let's see what @wing328 thinks of this.

@wing328
Copy link
Member

wing328 commented May 28, 2021

Let's go with this. If users prefer a way to make it backward compatible, we can introduce an option (but not something I prefer to avoid having too many options for a generator)

@4brunu
Copy link
Contributor

4brunu commented May 28, 2021

Looks good to me.
As @shanselm-ergon mentioned, the worth thing would be if someone uses named parameters, that would need to change this

ApiClient(..., okHttpClient = OkHttpClient(),  ...)

into this

ApiClient(..., callFactory = OkHttpClient(),  ...)

And if the users don't use named parameters, they don't need to change nothing.
@wing328 for me you can merge this PR. 👍

@wing328
Copy link
Member

wing328 commented May 28, 2021

Tested locally and the result is good:

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.8.3/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 2m 17s
3 actionable tasks: 3 executed
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 03:09 min
[INFO] Finished at: 2021-05-28T21:45:02+08:00
[INFO] ------------------------------------------------------------------------

@wing328 wing328 merged commit 1b6fd2d into OpenAPITools:master May 28, 2021
@wing328 wing328 added this to the 5.2.0 milestone May 28, 2021
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

3 participants