[kotlin] support selection of datelibrary #7054

magiepooh · 2017-11-25T08:23:05Z

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: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

support selection of date library. (java8, threetenbp, string)

# no option
ags="generate -t modules/swagger-codegen/src/main/resources/kotlin-client -i modules/swagger-codegen/src/test/resources/2_0/petstore.yaml -l kotlin --artifact-id kotlin-petstore-client -D -o samples/client/petstore/kotlin $@"

# java8
ags="generate -t modules/swagger-codegen/src/main/resources/kotlin-client -i modules/swagger-codegen/src/test/resources/2_0/petstore.yaml -l kotlin --artifact-id kotlin-petstore-client -D dateLibrary=java8 -o samples/client/petstore/kotlin $@"

# string
ags="generate -t modules/swagger-codegen/src/main/resources/kotlin-client -i modules/swagger-codegen/src/test/resources/2_0/petstore.yaml -l kotlin --artifact-id kotlin-petstore-client -D dateLibrary=string -o samples/client/petstore/kotlin $@"

# threetenbp
ags="generate -t modules/swagger-codegen/src/main/resources/kotlin-client -i modules/swagger-codegen/src/test/resources/2_0/petstore.yaml -l kotlin --artifact-id kotlin-petstore-client -D dateLibrary=threetenbp -o samples/client/petstore/kotlin $@"

related Issue: #6892

wing328 · 2017-11-27T04:10:46Z

@magiepooh thanks for the PR 👍

cc @jimschubert @cbornet

jimschubert

Thanks, this looks great!

I didn't run this to verify, but it looks reasonable and correct to me. Minor suggestion to change string "true" set on additionalProperties to true so templates can handle them as booleans. If they're required to be stringified, we should probably comment that where these are set.

jimschubert · 2017-11-27T04:44:22Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/KotlinClientCodegen.java

+            typeMapping.put("Date", "kotlin.String");
+            typeMapping.put("DateTime", "kotlin.String");
+        } else if (DateLibrary.JAVA8.value.equals(dateLibrary)) {
+            additionalProperties.put(DateLibrary.JAVA8.value, "true");


additionalProperties should be true here rather than "true" if it's intended to be used for switching in templates.

jimschubert · 2017-11-27T04:47:33Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/KotlinClientCodegen.java

+        }
+
+        if (DateLibrary.THREETENBP.value.equals(dateLibrary)) {
+            additionalProperties.put(DateLibrary.THREETENBP.value, "true");


additionalProperties should be true here rather than "true" if it's intended to be used for switching in templates.

jimschubert · 2017-11-27T04:47:35Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/KotlinClientCodegen.java

+            importMapping.put("LocalDate", "org.threeten.bp.LocalDate");
+            importMapping.put("LocalDateTime", "org.threeten.bp.LocalDateTime");
+        } else if (DateLibrary.STRING.value.equals(dateLibrary)) {
+            additionalProperties.put(DateLibrary.STRING.value, "true");


I think having just string as an additional property would be confusing.

For instance, in a template this would lead to:

{{#string}}string date logic{{/string}}{{^string}}other logic{{/string}}

This seems like an unlikely use case, until it starts getting used and we can't change it without a breaking change. Also, I think this may cause confusion on some property/params which may have string properties as well.

additionalProperties should be true here rather than "true" if it's intended to be used for switching in templates.

Any objection to using isStringDate or something? iirc, Mustache will consider the string "true" for properties prefixed with is as a boolean. I could be wrong, but I prefer to set booleans explicitly. At one point, we were able to set them as strings but I don't know if that code was changed or if the Mustache dependency was updated but as of a few weeks ago they had to be explicit booleans.

jimschubert · 2017-12-25T20:42:16Z

@magiepooh I don't know if you saw my comments? If you'd like, I can make the changes and open a PR against your fork.

magiepooh · 2018-01-05T00:51:55Z

Thank you for reviewing! (I was not aware. sorry.)

additionalProperties should be true here rather than "true" if it's intended to be used for switching in templates.

I'll fix it.

#7054 (comment)

I think that selection regarding dates should be managed with a unified option.

Multiple option (isStringDate option and dateLibrary option ) may confuse the user. What do you think?

jimschubert · 2018-01-06T15:59:31Z

@Maggiepooh I thought about it a little more. There's a isString and isDate on vars and params. Which of these would be true in this situation? I think it would be equally as confusing (maybe moreso) to have isString=false, isDate=true, and also have string=true available from additional properties. I think whatever is chosen will be confusing to consumers, mostly because String isn't a library for handling dates.

Maybe a better approach would be to set a private flag if the additional property is set (and remove it from additional properties to avoid any confusion), and post process variables and parameters to change from dates to strings. After all, that's what this option intends to do, correct? This way, it wouldn't interfere with templates that have isDate blocks.

I haven't looked at other generators to see if/how they handle this. @wing328 is there a common approach to forcing date/date-time to strings?

wing328 · 2018-01-09T10:00:01Z

For PHP, we use our own serializer to handle the datetime to string conversion. For Java, we rely on 3rd-party libraries.

For Kotlin, can we follow Java's approach? (just throwing a suggestion but I've not looked into the Kotlin client code)

magiepooh · 2018-01-15T12:15:28Z

I thought about it a little more. There's a isString and isDate on vars and params. Which of these would be true in this situation? I think it would be equally as confusing (maybe moreso) to have isString=false, isDate=true, and also have string=true available from additional properties. I think whatever is chosen will be confusing to consumers, mostly because String isn't a library for handling dates.

I agree.

@jimschubert Should I fix it as follows?
additionalProperties.put(DateLibrary.STRING.value, true);
↓
additionalProperties.put("isStringDate", true);

("isStringDate" may not need to be used. Other additional properties may use like this. )

swagger-codegen/modules/swagger-codegen/src/main/resources/Java/libraries/retrofit2/JSON.mustache

Line 15 in 73b38dd

magiepooh · 2018-01-19T06:49:08Z

@jimschubert
I added additionalProperties for retrofit (HTTP client for Android).
However, now I thought that the string property is not necessary when creating a Retrofit adapter. So, I removed the string property.

wing328 · 2018-01-20T06:44:15Z

@magiepooh CircleCI reports the following issue:

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':compileTestKotlin'.
> Could not resolve all dependencies for configuration ':testCompile'.
   > Could not resolve org.hamcrest:hamcrest-core:1.3.
     Required by:
         io.swagger:kotlin-petstore-client:1.0.0 > io.kotlintest:kotlintest:2.0.2 > com.sksamuel.koors:koors:0.90.0 > junit:junit:4.12
      > Could not HEAD 'http://repo1.maven.org/maven2/org/hamcrest/hamcrest-core/1.3/hamcrest-core-1.3.pom'.
         > Connection reset

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

BUILD FAILED

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

magiepooh · 2018-01-23T11:53:33Z

@wing328

Can you help me? I don't know why CI failure.
My change is only to fix additionalProperties. ( f39dc67...8d0f661 )

wing328 · 2018-01-25T14:33:01Z

I did a test with the option threetenbp but looks like there are some issues:

build.gradle does not have the following to import the module:

compile group: 'org.threeten', name: 'threetenbp', version: '1.3.6'

the following import statement is incorrect (in Order.kt)

import io.swagger.client.models.org.threeten.bp.LocalDateTime

I think I've solutions to these problems and will squeeze some time this weekend to work on it.

boylenssen · 2018-02-15T09:35:41Z

Why is legacy not supported here? For Java, this sets it to Date. I would like this option for Kotlin too.

I used the snapshot of 2.4.0 (feb 10) and threetenbp works fine there. Legacy gives me Java8 datetime

[kotlin] support selection of datelibrary

f39dc67

wing328 added the Client: Kotlin label Nov 27, 2017

wing328 added this to the v2.3.0 milestone Nov 27, 2017

wing328 added the Enhancement: General label Nov 27, 2017

jimschubert reviewed Nov 27, 2017

View reviewed changes

wing328 modified the milestones: v2.3.0, v2.4.0 Dec 18, 2017

magiepooh added 2 commits January 19, 2018 14:48

remove additional property from string

d6ba18c

replace string with boolean

8d0f661

Merge branch 'master' into fix_issue_6892

ffa74a1

wing328 merged commit 914275f into swagger-api:master Jan 28, 2018

wing328 mentioned this pull request Jan 28, 2018

[Kotlin] Fix issues with threetenbp #7516

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[kotlin] support selection of datelibrary #7054

[kotlin] support selection of datelibrary #7054

Uh oh!

magiepooh commented Nov 25, 2017 •

edited

Loading

Uh oh!

wing328 commented Nov 27, 2017

Uh oh!

jimschubert left a comment

Uh oh!

jimschubert Nov 27, 2017

Uh oh!

jimschubert Nov 27, 2017

Uh oh!

jimschubert Nov 27, 2017

Uh oh!

jimschubert commented Dec 25, 2017

Uh oh!

magiepooh commented Jan 5, 2018 •

edited

Loading

Uh oh!

jimschubert commented Jan 6, 2018

Uh oh!

wing328 commented Jan 9, 2018

Uh oh!

magiepooh commented Jan 15, 2018 •

edited

Loading

Uh oh!

magiepooh commented Jan 19, 2018 •

edited

Loading

Uh oh!

wing328 commented Jan 20, 2018

Uh oh!

magiepooh commented Jan 23, 2018 •

edited

Loading

Uh oh!

wing328 commented Jan 25, 2018

Uh oh!

boylenssen commented Feb 15, 2018

Uh oh!

Uh oh!

[kotlin] support selection of datelibrary #7054

[kotlin] support selection of datelibrary #7054

Uh oh!

Conversation

magiepooh commented Nov 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Description of the PR

Uh oh!

wing328 commented Nov 27, 2017

Uh oh!

jimschubert left a comment

Choose a reason for hiding this comment

Uh oh!

jimschubert Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

jimschubert Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

jimschubert Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

jimschubert commented Dec 25, 2017

Uh oh!

magiepooh commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimschubert commented Jan 6, 2018

Uh oh!

wing328 commented Jan 9, 2018

Uh oh!

magiepooh commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magiepooh commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wing328 commented Jan 20, 2018

Uh oh!

magiepooh commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wing328 commented Jan 25, 2018

Uh oh!

boylenssen commented Feb 15, 2018

Uh oh!

Uh oh!

magiepooh commented Nov 25, 2017 •

edited

Loading

magiepooh commented Jan 5, 2018 •

edited

Loading

magiepooh commented Jan 15, 2018 •

edited

Loading

magiepooh commented Jan 19, 2018 •

edited

Loading

magiepooh commented Jan 23, 2018 •

edited

Loading