Skip to content

Move all kotlin-client samples in subfolder kotlin #5718

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

Closed
wants to merge 12 commits into from
Closed

Move all kotlin-client samples in subfolder kotlin #5718

wants to merge 12 commits into from

Conversation

ch4rl3x
Copy link
Contributor

@ch4rl3x ch4rl3x commented Mar 26, 2020

Pre PR for Merging #5697

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.

PR Description

This PR moves all kotlin-client sample projects into subfolder named 'kotlin' for better merging and less diffs in PR #5697

@jimschubert @dr4ke616 @karismann @Zomzog @andrewemery @4brunu

@@ -0,0 +1,61 @@
package org.openapitools.client.auth
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are some files like this that don't belong to this PR.
Because the OAuth supports is still pending in the PR #5697
Can you please generate the sample projects again?

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 forgott to mvn clean install before rerun generate all sample projects. Fixed with a9a01a4

@@ -10,28 +10,28 @@ import okhttp3.MultipartBody
import org.openapitools.client.models.User

interface UserApi {
@POST("/user")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this change is right or not...
Can someone else give some feedback here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is still a bug in kotlin-retrofit2 generator. These releative pathes have to start WITHOUT slashes. See java-retrofit2 generator. I fixed it within my PR #5697
aaef017#diff-b60a03ff3897718b90d1c4d54850d0de in file KotlinClientCodegen.java

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate a bit more on why that's a bug?
I thought that "/user" was the correct behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/user was the behaviour before my PR #5697. Retrofit2 does not allow a baseURL without trailing slash. That means, the relative path has to start without slash. The current version of kotlin-retrofit2 didnt worked in our projekt. We checked the java-retrofit2 templates. The relative pathes in the api files in java-retrofit2 didnt start with slash.

We ran into following issue: square/retrofit#907

@4brunu
Copy link
Contributor

4brunu commented Mar 26, 2020

Looks good to me. 👍
Lets see what the other members of the technical committee think about this and if the CI passes.

@wing328
Copy link
Member

wing328 commented Mar 26, 2020

I think you will also need to update

openapi-generator/pom.xml

Lines 1451 to 1461 in a9a01a4

<module>samples/client/petstore/kotlin-multiplatform</module>
<module>samples/client/petstore/kotlin/</module>
<module>samples/client/petstore/kotlin-jackson/</module>
<module>samples/client/petstore/kotlin-gson/</module>
<module>samples/client/petstore/kotlin-nonpublic/</module>
<module>samples/client/petstore/kotlin-nullable/</module>
<module>samples/client/petstore/kotlin-okhttp3/</module>
<module>samples/client/petstore/kotlin-threetenbp/</module>
<module>samples/client/petstore/kotlin-string/</module>
<module>samples/client/petstore/kotlin-moshi-codegen/</module>
<module>samples/client/petstore/kotlin-json-request-string/</module>

@ch4rl3x
Copy link
Contributor Author

ch4rl3x commented Mar 26, 2020

I think you will also need to update

openapi-generator/pom.xml

Lines 1451 to 1461 in a9a01a4

<module>samples/client/petstore/kotlin-multiplatform</module>
<module>samples/client/petstore/kotlin/</module>
<module>samples/client/petstore/kotlin-jackson/</module>
<module>samples/client/petstore/kotlin-gson/</module>
<module>samples/client/petstore/kotlin-nonpublic/</module>
<module>samples/client/petstore/kotlin-nullable/</module>
<module>samples/client/petstore/kotlin-okhttp3/</module>
<module>samples/client/petstore/kotlin-threetenbp/</module>
<module>samples/client/petstore/kotlin-string/</module>
<module>samples/client/petstore/kotlin-moshi-codegen/</module>
<module>samples/client/petstore/kotlin-json-request-string/</module>

Should I edit the file manually or is it generated by a shell script

@wing328
Copy link
Member

wing328 commented Mar 26, 2020

That file is manually maintained

@ch4rl3x
Copy link
Contributor Author

ch4rl3x commented Mar 26, 2020

I updated the pom.xml in root path, but still errors in Shippable Ci. Missing pom.xml files in all kotlin projects. Anything to todo?

@wing328
Copy link
Member

wing328 commented Mar 26, 2020

We manually added a (unique) pom.xml file in the Kotlin sample folder. You will need to move the pom.xml to the new directory in order for maven to run the integration tests

@ch4rl3x
Copy link
Contributor Author

ch4rl3x commented Apr 7, 2020

Anything to do? Don't know why these ci builds failed

@4brunu
Copy link
Contributor

4brunu commented Apr 7, 2020

I don't know what's the problem, but looking at the travis config file,
https://github.com/OpenAPITools/openapi-generator/blob/master/.travis.yml#L144
It points to
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator-maven-plugin/examples/kotlin.xml

Maybe something needs to be updated there?
I'm not sure.
@wing328 do you have any tips on how to fix travis?

@ch4rl3x ch4rl3x requested a review from jimschubert as a code owner April 8, 2020 11:10
@ch4rl3x ch4rl3x closed this Aug 31, 2020
@ch4rl3x ch4rl3x deleted the kotlin-sample-projects-in-sub-folder branch August 31, 2020 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants