Skip to content

New generator: kotlin-retrofit (lightweight, Android, coroutines) - #3195

Closed
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom
kroegerama:kotlin-retrofit
Closed

New generator: kotlin-retrofit (lightweight, Android, coroutines)#3195
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom
kroegerama:kotlin-retrofit

Conversation

@kroegerama

@kroegeramakroegerama commented Jun 21, 2019

Copy link
Copy Markdown
Contributor

See #3135

@kroegerama
add new generator: kotlin-retrofit
23d5d5e
@kroegerama
Merge branch 'master' ofhttps://github.com/OpenAPITools/openapi-gene…
67686e3
…rator into kotlin-retrofit
@kroegerama
improve generated code quality
9f49935
* KotlinRetrofitCodegen no more uses AbstractKotlinCodegen (has wrong reserved word list)
* support for default values for Api parameters
@kroegerama
add support for allOf (inheritance)
1e36f85
@kroegerama
Merge pull request#1from OpenAPITools/master
80aa5b4
merge latest master
@auto-labeler

auto-labelerBot commented Jun 21, 2019

Copy link
Copy Markdown

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@wing328

wing328 commented Jun 24, 2019

Copy link
Copy Markdown
Member

@kroegerama thanks for contributing a new generator.

Have you considered incorporating the retrofit support via a --library option to the existing kotlin client generator?

The current java client generator supports many HTTP libraries via the --library option.

@wing328wing328 added this to the 4.0.3 milestone Jun 24, 2019
@wing328

wing328 commented Jun 24, 2019

Copy link
Copy Markdown
Member

cc Kotlin tech committe: @jimschubert (2017/09) , @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04)

@wing328wing328 modified the milestones: 4.0.3, 4.1.0Jul 9, 2019
MatkovIvan
MatkovIvan reviewed Jul 19, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java

import static org.openapitools.codegen.utils.StringUtils.*;

public class KotlinRetrofitCodegen extends DefaultCodegen implements CodegenConfig {

@MatkovIvanMatkovIvanJul 19, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I guess AbstractKotlinCodegen should be used here

@kroegeramakroegeramaJul 19, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Nope. AbstractKotlinCodegen has some wrong implementations. See my explanation here which is just one error I found in the abstract class.
We could merge the corrections in my Codegen into AbstractKotlinCodegen, though.

@MatkovIvanMatkovIvanJul 19, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fix found error in AbstractKotlinCodegen is the right option. Plus, it can be done in separate PR to make contributions smaller -> get reviews easier -> merge faster.

BTW it's a nice contribution, looking forward to it 👍

@wing328wing328Jul 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed with @MatkovIvan to fix the bug in AbstractKotlinCodegen instead.

@snackycracky

snackycracky commented Aug 4, 2019

Copy link
Copy Markdown

hey, thanks for the effort. Would be awesome if any kind of authenticationScheme is supported :)

@wing328wing328 modified the milestones: 4.1.0, 4.1.1Aug 9, 2019
@wing328wing328 modified the milestones: 4.1.1, 4.1.2Aug 26, 2019
@jakoss

jakoss commented Sep 6, 2019

Copy link
Copy Markdown

Hey. Great idea you have there. Do you have any ETA on release? Maybe i could somehow help push this further?

@wing328wing328 modified the milestones: 4.1.2, 4.1.3Sep 11, 2019
@craigmurray1120

craigmurray1120 commented Sep 20, 2019

Copy link
Copy Markdown

Would love to see this generator gets merged into master.

Thank you @kroegerama for his awesome work.

@kroegerama
Merge pull request#2from OpenAPITools/master
80ef77b
merge from OpenAPITools/openapi-generator master
@wing328wing328 modified the milestones: 4.1.3, 4.2.0Oct 4, 2019
@wing328wing328 removed this from the 4.2.0 milestone Oct 30, 2019
@kroegerama
improve performance, hide debug info
4db0206
* add retrofit scalars converter for simple types
* only add HttpLoggingInterceptor in Debug builds
* initialize OkHttpClient lazy on non-main thread (see https://www.zacsweers.dev/dagger-party-tricks-deferred-okhttp-init/)
@kroegerama
update okHttp version
735ed18
@kroegerama
add api version constant to RetrofitHolder
5cc0063
@kroegerama
add versionName to build.gradle
55b443d
jimschubert
jimschubert reviewed Nov 18, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
setCollectionType(additionalProperties.get(COLLECTION_TYPE).toString());
}

if (CollectionType.LIST.value.equals(collectionType)) {

@jimschubertjimschubertNov 14, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should have a warning that we're modifying typeMappings at this point, because it'll beak user's ability to define type mappings when using generator defaults.

Comment threadmodules/openapi-generator/src/main/resources/kotlin-retrofit/api.mustache
import retrofit2.create
import retrofit2.http.*
import okhttp3.*
import retrofit2.http.Headers

@jimschubertjimschubertNov 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this necessary with the glob import above?

Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
supportingFiles.add(new SupportingFile("proguard-rules.pro.mustache", "", "proguard-rules.pro"));
supportingFiles.add(new SupportingFile("gitignore.mustache", "", ".gitignore"));

supportingFiles.add(new SupportingFile("AndroidManifest.xml.mustache", "src/main", "AndroidManifest.xml"));

@jimschubertjimschubertNov 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note: we may want to put Android options behind a CLI option. I scanned and didn't see any Android specific code in client files.

@jimschubert

jimschubert commented Nov 18, 2019

Copy link
Copy Markdown
Member

I agree that this would make more sense as a library option to the existing client generator. It seems that one of the major reasons it wasn't was because some keywords (like data) are set as reserved but disallowed for variables. Many of these aren't allowed elsewhere (can't recall exactly, but I think as Enums), so I think any special casing logic makes the most sense in the base class to benefit all Kotlin generators.

What might you need from the core team to get this into master? Maybe we could do it as a separate Beta generator and move to a library option later?

@wing328

wing328 commented Dec 14, 2019

Copy link
Copy Markdown
Member

Here is another PR to add retrofit2 as a library option to the Kotlin client generator: #4518

@wing328

wing328 commented Dec 22, 2019

Copy link
Copy Markdown
Member

#4518 has been merged into master.

Please give it a try. Closing this one for the time being.

@wing328wing328 closed this Dec 22, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimschubertjimschubertjimschubert left review comments
@wing328wing328wing328 left review comments
+1 more reviewer
@MatkovIvanMatkovIvanMatkovIvan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Client: KotlinClient: RetrofitEnhancement: New generator

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@kroegerama@wing328@snackycracky@jakoss@craigmurray1120@jimschubert@MatkovIvan
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
New generator: kotlin-retrofit (lightweight, Android, coroutines) by kroegerama · Pull Request #3195 · OpenAPITools/openapi-generator · GitHub
Skip to content
New generator: kotlin-retrofit (lightweight, Android, coroutines) - #3195
Closed
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Closed
New generator: kotlin-retrofit (lightweight, Android, coroutines)#3195
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Conversation
@kroegerama
@kroegeramakroegerama
commented
Jun 21, 2019

Copy link


Copy Markdown

Contributor
See #3135
@kroegerama
add new generator: kotlin-retrofit
23d5d5e
@kroegerama
Merge branch 'master' ofhttps://github.com/OpenAPITools/openapi-gene…
67686e3
…rator into kotlin-retrofit
@kroegerama
improve generated code quality
9f49935
* KotlinRetrofitCodegen no more uses AbstractKotlinCodegen (has wrong reserved word list)
* support for default values for Api parameters
@kroegerama
add support for allOf (inheritance)
1e36f85
@kroegerama
Merge pull request#1from OpenAPITools/master
80aa5b4
merge latest master
@auto-labeler
auto-labelerBot
commented
Jun 21, 2019

Copy link


Copy Markdown

👍 Thanks for opening this issue!

🏷 I have applied any labels matching special text in your issue.
The team will review the labels and make any necessary changes.
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
@kroegerama thanks for contributing a new generator.
Have you considered incorporating the retrofit support via a --library option to the existing kotlin client generator?
The current java client generator supports many HTTP libraries via the --library option.
@wing328wing328
added this to the 4.0.3 milestone
Jun 24, 2019
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
cc Kotlin tech committe: @jimschubert (2017/09) , @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04)
@wing328wing328
modified the milestones:
4.0.3, 4.1.0Jul 9, 2019
MatkovIvan
MatkovIvan
reviewed
Jul 19, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java

import static org.openapitools.codegen.utils.StringUtils.*;

public class KotlinRetrofitCodegen extends DefaultCodegen implements CodegenConfig {
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I guess AbstractKotlinCodegen should be used here
@kroegeramakroegeramaJul 19, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Nope. AbstractKotlinCodegen has some wrong implementations. See my explanation here which is just one error I found in the abstract class.

We could merge the corrections in my Codegen into AbstractKotlinCodegen, though.
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Fix found error in AbstractKotlinCodegen is the right option. Plus, it can be done in separate PR to make contributions smaller -> get reviews easier -> merge faster.
BTW it's a nice contribution, looking forward to it 👍
@wing328wing328Jul 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Agreed with @MatkovIvan to fix the bug in AbstractKotlinCodegen instead.
@snackycracky
snackycracky
commented
Aug 4, 2019

Copy link


Copy Markdown

hey, thanks for the effort. Would be awesome if any kind of authenticationScheme is supported :)
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@jakoss
jakoss
commented
Sep 6, 2019

Copy link


Copy Markdown

Hey. Great idea you have there. Do you have any ETA on release? Maybe i could somehow help push this further?
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@craigmurray1120
craigmurray1120
commented
Sep 20, 2019

Copy link


Copy Markdown

Would love to see this generator gets merged into master.
Thank you @kroegerama for his awesome work.
@kroegerama
Merge pull request#2from OpenAPITools/master
80ef77b
merge from OpenAPITools/openapi-generator master
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
@kroegerama
improve performance, hide debug info
4db0206
* add retrofit scalars converter for simple types
* only add HttpLoggingInterceptor in Debug builds
* initialize OkHttpClient lazy on non-main thread (see https://www.zacsweers.dev/dagger-party-tricks-deferred-okhttp-init/)
@kroegerama
update okHttp version
735ed18
@kroegerama
add api version constant to RetrofitHolder
5cc0063
@kroegerama
add versionName to build.gradle
55b443d
jimschubert
jimschubert
reviewed
Nov 18, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 setCollectionType(additionalProperties.get(COLLECTION_TYPE).toString());
 }

 if (CollectionType.LIST.value.equals(collectionType)) {
@jimschubertjimschubertNov 14, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This should have a warning that we're modifying typeMappings at this point, because it'll beak user's ability to define type mappings when using generator defaults.
Comment threadmodules/openapi-generator/src/main/resources/kotlin-retrofit/api.mustache
import retrofit2.create
import retrofit2.http.*
import okhttp3.*
import retrofit2.http.Headers
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Is this necessary with the glob import above?
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 supportingFiles.add(new SupportingFile("proguard-rules.pro.mustache", "", "proguard-rules.pro"));
 supportingFiles.add(new SupportingFile("gitignore.mustache", "", ".gitignore"));

 supportingFiles.add(new SupportingFile("AndroidManifest.xml.mustache", "src/main", "AndroidManifest.xml"));
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Note: we may want to put Android options behind a CLI option. I scanned and didn't see any Android specific code in client files.
@jimschubert
jimschubert
commented
Nov 18, 2019

Copy link


Copy Markdown

Member
I agree that this would make more sense as a library option to the existing client generator. It seems that one of the major reasons it wasn't was because some keywords (like data) are set as reserved but disallowed for variables. Many of these aren't allowed elsewhere (can't recall exactly, but I think as Enums), so I think any special casing logic makes the most sense in the base class to benefit all Kotlin generators.
What might you need from the core team to get this into master? Maybe we could do it as a separate Beta generator and move to a library option later?
@wing328
wing328
commented
Dec 14, 2019

Copy link


Copy Markdown

Member
Here is another PR to add retrofit2 as a library option to the Kotlin client generator: #4518
@wing328
wing328
commented
Dec 22, 2019

Copy link


Copy Markdown

Member
#4518 has been merged into master.
Please give it a try. Closing this one for the time being.
@wing328wing328
closed this
Dec 22, 2019
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jimschubertjimschubertjimschubert left review comments
@wing328wing328wing328 left review comments

+1 more reviewer

@MatkovIvanMatkovIvanMatkovIvan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Client: KotlinClient: RetrofitEnhancement: New generator

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@kroegerama@wing328@snackycracky@jakoss@craigmurray1120@jimschubert@MatkovIvan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
New generator: kotlin-retrofit (lightweight, Android, coroutines) by kroegerama · Pull Request #3195 · OpenAPITools/openapi-generator · GitHub
Skip to content
New generator: kotlin-retrofit (lightweight, Android, coroutines) - #3195
Closed
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Closed
New generator: kotlin-retrofit (lightweight, Android, coroutines)#3195
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Conversation
@kroegerama
@kroegeramakroegerama
commented
Jun 21, 2019

Copy link


Copy Markdown

Contributor
See #3135
@kroegerama
add new generator: kotlin-retrofit
23d5d5e
@kroegerama
Merge branch 'master' ofhttps://github.com/OpenAPITools/openapi-gene…
67686e3
…rator into kotlin-retrofit
@kroegerama
improve generated code quality
9f49935
* KotlinRetrofitCodegen no more uses AbstractKotlinCodegen (has wrong reserved word list)
* support for default values for Api parameters
@kroegerama
add support for allOf (inheritance)
1e36f85
@kroegerama
Merge pull request#1from OpenAPITools/master
80aa5b4
merge latest master
@auto-labeler
auto-labelerBot
commented
Jun 21, 2019

Copy link


Copy Markdown

👍 Thanks for opening this issue!

🏷 I have applied any labels matching special text in your issue.
The team will review the labels and make any necessary changes.
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
@kroegerama thanks for contributing a new generator.
Have you considered incorporating the retrofit support via a --library option to the existing kotlin client generator?
The current java client generator supports many HTTP libraries via the --library option.
@wing328wing328
added this to the 4.0.3 milestone
Jun 24, 2019
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
cc Kotlin tech committe: @jimschubert (2017/09) , @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04)
@wing328wing328
modified the milestones:
4.0.3, 4.1.0Jul 9, 2019
MatkovIvan
MatkovIvan
reviewed
Jul 19, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java

import static org.openapitools.codegen.utils.StringUtils.*;

public class KotlinRetrofitCodegen extends DefaultCodegen implements CodegenConfig {
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I guess AbstractKotlinCodegen should be used here
@kroegeramakroegeramaJul 19, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Nope. AbstractKotlinCodegen has some wrong implementations. See my explanation here which is just one error I found in the abstract class.

We could merge the corrections in my Codegen into AbstractKotlinCodegen, though.
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Fix found error in AbstractKotlinCodegen is the right option. Plus, it can be done in separate PR to make contributions smaller -> get reviews easier -> merge faster.
BTW it's a nice contribution, looking forward to it 👍
@wing328wing328Jul 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Agreed with @MatkovIvan to fix the bug in AbstractKotlinCodegen instead.
@snackycracky
snackycracky
commented
Aug 4, 2019

Copy link


Copy Markdown

hey, thanks for the effort. Would be awesome if any kind of authenticationScheme is supported :)
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@jakoss
jakoss
commented
Sep 6, 2019

Copy link


Copy Markdown

Hey. Great idea you have there. Do you have any ETA on release? Maybe i could somehow help push this further?
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@craigmurray1120
craigmurray1120
commented
Sep 20, 2019

Copy link


Copy Markdown

Would love to see this generator gets merged into master.
Thank you @kroegerama for his awesome work.
@kroegerama
Merge pull request#2from OpenAPITools/master
80ef77b
merge from OpenAPITools/openapi-generator master
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
@kroegerama
improve performance, hide debug info
4db0206
* add retrofit scalars converter for simple types
* only add HttpLoggingInterceptor in Debug builds
* initialize OkHttpClient lazy on non-main thread (see https://www.zacsweers.dev/dagger-party-tricks-deferred-okhttp-init/)
@kroegerama
update okHttp version
735ed18
@kroegerama
add api version constant to RetrofitHolder
5cc0063
@kroegerama
add versionName to build.gradle
55b443d
jimschubert
jimschubert
reviewed
Nov 18, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 setCollectionType(additionalProperties.get(COLLECTION_TYPE).toString());
 }

 if (CollectionType.LIST.value.equals(collectionType)) {
@jimschubertjimschubertNov 14, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This should have a warning that we're modifying typeMappings at this point, because it'll beak user's ability to define type mappings when using generator defaults.
Comment threadmodules/openapi-generator/src/main/resources/kotlin-retrofit/api.mustache
import retrofit2.create
import retrofit2.http.*
import okhttp3.*
import retrofit2.http.Headers
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Is this necessary with the glob import above?
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 supportingFiles.add(new SupportingFile("proguard-rules.pro.mustache", "", "proguard-rules.pro"));
 supportingFiles.add(new SupportingFile("gitignore.mustache", "", ".gitignore"));

 supportingFiles.add(new SupportingFile("AndroidManifest.xml.mustache", "src/main", "AndroidManifest.xml"));
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Note: we may want to put Android options behind a CLI option. I scanned and didn't see any Android specific code in client files.
@jimschubert
jimschubert
commented
Nov 18, 2019

Copy link


Copy Markdown

Member
I agree that this would make more sense as a library option to the existing client generator. It seems that one of the major reasons it wasn't was because some keywords (like data) are set as reserved but disallowed for variables. Many of these aren't allowed elsewhere (can't recall exactly, but I think as Enums), so I think any special casing logic makes the most sense in the base class to benefit all Kotlin generators.
What might you need from the core team to get this into master? Maybe we could do it as a separate Beta generator and move to a library option later?
@wing328
wing328
commented
Dec 14, 2019

Copy link


Copy Markdown

Member
Here is another PR to add retrofit2 as a library option to the Kotlin client generator: #4518
@wing328
wing328
commented
Dec 22, 2019

Copy link


Copy Markdown

Member
#4518 has been merged into master.
Please give it a try. Closing this one for the time being.
@wing328wing328
closed this
Dec 22, 2019
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jimschubertjimschubertjimschubert left review comments
@wing328wing328wing328 left review comments

+1 more reviewer

@MatkovIvanMatkovIvanMatkovIvan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Client: KotlinClient: RetrofitEnhancement: New generator

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@kroegerama@wing328@snackycracky@jakoss@craigmurray1120@jimschubert@MatkovIvan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
New generator: kotlin-retrofit (lightweight, Android, coroutines) by kroegerama · Pull Request #3195 · OpenAPITools/openapi-generator · GitHub
Skip to content
New generator: kotlin-retrofit (lightweight, Android, coroutines) - #3195
Closed
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Closed
New generator: kotlin-retrofit (lightweight, Android, coroutines)#3195
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Conversation
@kroegerama
@kroegeramakroegerama
commented
Jun 21, 2019

Copy link


Copy Markdown

Contributor
See #3135
@kroegerama
add new generator: kotlin-retrofit
23d5d5e
@kroegerama
Merge branch 'master' ofhttps://github.com/OpenAPITools/openapi-gene…
67686e3
…rator into kotlin-retrofit
@kroegerama
improve generated code quality
9f49935
* KotlinRetrofitCodegen no more uses AbstractKotlinCodegen (has wrong reserved word list)
* support for default values for Api parameters
@kroegerama
add support for allOf (inheritance)
1e36f85
@kroegerama
Merge pull request#1from OpenAPITools/master
80aa5b4
merge latest master
@auto-labeler
auto-labelerBot
commented
Jun 21, 2019

Copy link


Copy Markdown

👍 Thanks for opening this issue!

🏷 I have applied any labels matching special text in your issue.
The team will review the labels and make any necessary changes.
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
@kroegerama thanks for contributing a new generator.
Have you considered incorporating the retrofit support via a --library option to the existing kotlin client generator?
The current java client generator supports many HTTP libraries via the --library option.
@wing328wing328
added this to the 4.0.3 milestone
Jun 24, 2019
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
cc Kotlin tech committe: @jimschubert (2017/09) , @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04)
@wing328wing328
modified the milestones:
4.0.3, 4.1.0Jul 9, 2019
MatkovIvan
MatkovIvan
reviewed
Jul 19, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java

import static org.openapitools.codegen.utils.StringUtils.*;

public class KotlinRetrofitCodegen extends DefaultCodegen implements CodegenConfig {
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I guess AbstractKotlinCodegen should be used here
@kroegeramakroegeramaJul 19, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Nope. AbstractKotlinCodegen has some wrong implementations. See my explanation here which is just one error I found in the abstract class.

We could merge the corrections in my Codegen into AbstractKotlinCodegen, though.
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Fix found error in AbstractKotlinCodegen is the right option. Plus, it can be done in separate PR to make contributions smaller -> get reviews easier -> merge faster.
BTW it's a nice contribution, looking forward to it 👍
@wing328wing328Jul 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Agreed with @MatkovIvan to fix the bug in AbstractKotlinCodegen instead.
@snackycracky
snackycracky
commented
Aug 4, 2019

Copy link


Copy Markdown

hey, thanks for the effort. Would be awesome if any kind of authenticationScheme is supported :)
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@jakoss
jakoss
commented
Sep 6, 2019

Copy link


Copy Markdown

Hey. Great idea you have there. Do you have any ETA on release? Maybe i could somehow help push this further?
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@craigmurray1120
craigmurray1120
commented
Sep 20, 2019

Copy link


Copy Markdown

Would love to see this generator gets merged into master.
Thank you @kroegerama for his awesome work.
@kroegerama
Merge pull request#2from OpenAPITools/master
80ef77b
merge from OpenAPITools/openapi-generator master
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
@kroegerama
improve performance, hide debug info
4db0206
* add retrofit scalars converter for simple types
* only add HttpLoggingInterceptor in Debug builds
* initialize OkHttpClient lazy on non-main thread (see https://www.zacsweers.dev/dagger-party-tricks-deferred-okhttp-init/)
@kroegerama
update okHttp version
735ed18
@kroegerama
add api version constant to RetrofitHolder
5cc0063
@kroegerama
add versionName to build.gradle
55b443d
jimschubert
jimschubert
reviewed
Nov 18, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 setCollectionType(additionalProperties.get(COLLECTION_TYPE).toString());
 }

 if (CollectionType.LIST.value.equals(collectionType)) {
@jimschubertjimschubertNov 14, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This should have a warning that we're modifying typeMappings at this point, because it'll beak user's ability to define type mappings when using generator defaults.
Comment threadmodules/openapi-generator/src/main/resources/kotlin-retrofit/api.mustache
import retrofit2.create
import retrofit2.http.*
import okhttp3.*
import retrofit2.http.Headers
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Is this necessary with the glob import above?
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 supportingFiles.add(new SupportingFile("proguard-rules.pro.mustache", "", "proguard-rules.pro"));
 supportingFiles.add(new SupportingFile("gitignore.mustache", "", ".gitignore"));

 supportingFiles.add(new SupportingFile("AndroidManifest.xml.mustache", "src/main", "AndroidManifest.xml"));
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Note: we may want to put Android options behind a CLI option. I scanned and didn't see any Android specific code in client files.
@jimschubert
jimschubert
commented
Nov 18, 2019

Copy link


Copy Markdown

Member
I agree that this would make more sense as a library option to the existing client generator. It seems that one of the major reasons it wasn't was because some keywords (like data) are set as reserved but disallowed for variables. Many of these aren't allowed elsewhere (can't recall exactly, but I think as Enums), so I think any special casing logic makes the most sense in the base class to benefit all Kotlin generators.
What might you need from the core team to get this into master? Maybe we could do it as a separate Beta generator and move to a library option later?
@wing328
wing328
commented
Dec 14, 2019

Copy link


Copy Markdown

Member
Here is another PR to add retrofit2 as a library option to the Kotlin client generator: #4518
@wing328
wing328
commented
Dec 22, 2019

Copy link


Copy Markdown

Member
#4518 has been merged into master.
Please give it a try. Closing this one for the time being.
@wing328wing328
closed this
Dec 22, 2019
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jimschubertjimschubertjimschubert left review comments
@wing328wing328wing328 left review comments

+1 more reviewer

@MatkovIvanMatkovIvanMatkovIvan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Client: KotlinClient: RetrofitEnhancement: New generator

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@kroegerama@wing328@snackycracky@jakoss@craigmurray1120@jimschubert@MatkovIvan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
New generator: kotlin-retrofit (lightweight, Android, coroutines) by kroegerama · Pull Request #3195 · OpenAPITools/openapi-generator · GitHub
Skip to content
New generator: kotlin-retrofit (lightweight, Android, coroutines) - #3195
Closed
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Closed
New generator: kotlin-retrofit (lightweight, Android, coroutines)#3195
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Conversation
@kroegerama
@kroegeramakroegerama
commented
Jun 21, 2019

Copy link


Copy Markdown

Contributor
See #3135
@kroegerama
add new generator: kotlin-retrofit
23d5d5e
@kroegerama
Merge branch 'master' ofhttps://github.com/OpenAPITools/openapi-gene…
67686e3
…rator into kotlin-retrofit
@kroegerama
improve generated code quality
9f49935
* KotlinRetrofitCodegen no more uses AbstractKotlinCodegen (has wrong reserved word list)
* support for default values for Api parameters
@kroegerama
add support for allOf (inheritance)
1e36f85
@kroegerama
Merge pull request#1from OpenAPITools/master
80aa5b4
merge latest master
@auto-labeler
auto-labelerBot
commented
Jun 21, 2019

Copy link


Copy Markdown

👍 Thanks for opening this issue!

🏷 I have applied any labels matching special text in your issue.
The team will review the labels and make any necessary changes.
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
@kroegerama thanks for contributing a new generator.
Have you considered incorporating the retrofit support via a --library option to the existing kotlin client generator?
The current java client generator supports many HTTP libraries via the --library option.
@wing328wing328
added this to the 4.0.3 milestone
Jun 24, 2019
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
cc Kotlin tech committe: @jimschubert (2017/09) , @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04)
@wing328wing328
modified the milestones:
4.0.3, 4.1.0Jul 9, 2019
MatkovIvan
MatkovIvan
reviewed
Jul 19, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java

import static org.openapitools.codegen.utils.StringUtils.*;

public class KotlinRetrofitCodegen extends DefaultCodegen implements CodegenConfig {
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I guess AbstractKotlinCodegen should be used here
@kroegeramakroegeramaJul 19, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Nope. AbstractKotlinCodegen has some wrong implementations. See my explanation here which is just one error I found in the abstract class.

We could merge the corrections in my Codegen into AbstractKotlinCodegen, though.
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Fix found error in AbstractKotlinCodegen is the right option. Plus, it can be done in separate PR to make contributions smaller -> get reviews easier -> merge faster.
BTW it's a nice contribution, looking forward to it 👍
@wing328wing328Jul 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Agreed with @MatkovIvan to fix the bug in AbstractKotlinCodegen instead.
@snackycracky
snackycracky
commented
Aug 4, 2019

Copy link


Copy Markdown

hey, thanks for the effort. Would be awesome if any kind of authenticationScheme is supported :)
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@jakoss
jakoss
commented
Sep 6, 2019

Copy link


Copy Markdown

Hey. Great idea you have there. Do you have any ETA on release? Maybe i could somehow help push this further?
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@craigmurray1120
craigmurray1120
commented
Sep 20, 2019

Copy link


Copy Markdown

Would love to see this generator gets merged into master.
Thank you @kroegerama for his awesome work.
@kroegerama
Merge pull request#2from OpenAPITools/master
80ef77b
merge from OpenAPITools/openapi-generator master
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
@kroegerama
improve performance, hide debug info
4db0206
* add retrofit scalars converter for simple types
* only add HttpLoggingInterceptor in Debug builds
* initialize OkHttpClient lazy on non-main thread (see https://www.zacsweers.dev/dagger-party-tricks-deferred-okhttp-init/)
@kroegerama
update okHttp version
735ed18
@kroegerama
add api version constant to RetrofitHolder
5cc0063
@kroegerama
add versionName to build.gradle
55b443d
jimschubert
jimschubert
reviewed
Nov 18, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 setCollectionType(additionalProperties.get(COLLECTION_TYPE).toString());
 }

 if (CollectionType.LIST.value.equals(collectionType)) {
@jimschubertjimschubertNov 14, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This should have a warning that we're modifying typeMappings at this point, because it'll beak user's ability to define type mappings when using generator defaults.
Comment threadmodules/openapi-generator/src/main/resources/kotlin-retrofit/api.mustache
import retrofit2.create
import retrofit2.http.*
import okhttp3.*
import retrofit2.http.Headers
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Is this necessary with the glob import above?
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 supportingFiles.add(new SupportingFile("proguard-rules.pro.mustache", "", "proguard-rules.pro"));
 supportingFiles.add(new SupportingFile("gitignore.mustache", "", ".gitignore"));

 supportingFiles.add(new SupportingFile("AndroidManifest.xml.mustache", "src/main", "AndroidManifest.xml"));
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Note: we may want to put Android options behind a CLI option. I scanned and didn't see any Android specific code in client files.
@jimschubert
jimschubert
commented
Nov 18, 2019

Copy link


Copy Markdown

Member
I agree that this would make more sense as a library option to the existing client generator. It seems that one of the major reasons it wasn't was because some keywords (like data) are set as reserved but disallowed for variables. Many of these aren't allowed elsewhere (can't recall exactly, but I think as Enums), so I think any special casing logic makes the most sense in the base class to benefit all Kotlin generators.
What might you need from the core team to get this into master? Maybe we could do it as a separate Beta generator and move to a library option later?
@wing328
wing328
commented
Dec 14, 2019

Copy link


Copy Markdown

Member
Here is another PR to add retrofit2 as a library option to the Kotlin client generator: #4518
@wing328
wing328
commented
Dec 22, 2019

Copy link


Copy Markdown

Member
#4518 has been merged into master.
Please give it a try. Closing this one for the time being.
@wing328wing328
closed this
Dec 22, 2019
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jimschubertjimschubertjimschubert left review comments
@wing328wing328wing328 left review comments

+1 more reviewer

@MatkovIvanMatkovIvanMatkovIvan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Client: KotlinClient: RetrofitEnhancement: New generator

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@kroegerama@wing328@snackycracky@jakoss@craigmurray1120@jimschubert@MatkovIvan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
New generator: kotlin-retrofit (lightweight, Android, coroutines) by kroegerama · Pull Request #3195 · OpenAPITools/openapi-generator · GitHub
Skip to content
New generator: kotlin-retrofit (lightweight, Android, coroutines) - #3195
Closed
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Closed
New generator: kotlin-retrofit (lightweight, Android, coroutines)#3195
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Conversation
@kroegerama
@kroegeramakroegerama
commented
Jun 21, 2019

Copy link


Copy Markdown

Contributor
See #3135
@kroegerama
add new generator: kotlin-retrofit
23d5d5e
@kroegerama
Merge branch 'master' ofhttps://github.com/OpenAPITools/openapi-gene…
67686e3
…rator into kotlin-retrofit
@kroegerama
improve generated code quality
9f49935
* KotlinRetrofitCodegen no more uses AbstractKotlinCodegen (has wrong reserved word list)
* support for default values for Api parameters
@kroegerama
add support for allOf (inheritance)
1e36f85
@kroegerama
Merge pull request#1from OpenAPITools/master
80aa5b4
merge latest master
@auto-labeler
auto-labelerBot
commented
Jun 21, 2019

Copy link


Copy Markdown

👍 Thanks for opening this issue!

🏷 I have applied any labels matching special text in your issue.
The team will review the labels and make any necessary changes.
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
@kroegerama thanks for contributing a new generator.
Have you considered incorporating the retrofit support via a --library option to the existing kotlin client generator?
The current java client generator supports many HTTP libraries via the --library option.
@wing328wing328
added this to the 4.0.3 milestone
Jun 24, 2019
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
cc Kotlin tech committe: @jimschubert (2017/09) , @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04)
@wing328wing328
modified the milestones:
4.0.3, 4.1.0Jul 9, 2019
MatkovIvan
MatkovIvan
reviewed
Jul 19, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java

import static org.openapitools.codegen.utils.StringUtils.*;

public class KotlinRetrofitCodegen extends DefaultCodegen implements CodegenConfig {
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I guess AbstractKotlinCodegen should be used here
@kroegeramakroegeramaJul 19, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Nope. AbstractKotlinCodegen has some wrong implementations. See my explanation here which is just one error I found in the abstract class.

We could merge the corrections in my Codegen into AbstractKotlinCodegen, though.
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Fix found error in AbstractKotlinCodegen is the right option. Plus, it can be done in separate PR to make contributions smaller -> get reviews easier -> merge faster.
BTW it's a nice contribution, looking forward to it 👍
@wing328wing328Jul 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Agreed with @MatkovIvan to fix the bug in AbstractKotlinCodegen instead.
@snackycracky
snackycracky
commented
Aug 4, 2019

Copy link


Copy Markdown

hey, thanks for the effort. Would be awesome if any kind of authenticationScheme is supported :)
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@jakoss
jakoss
commented
Sep 6, 2019

Copy link


Copy Markdown

Hey. Great idea you have there. Do you have any ETA on release? Maybe i could somehow help push this further?
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@craigmurray1120
craigmurray1120
commented
Sep 20, 2019

Copy link


Copy Markdown

Would love to see this generator gets merged into master.
Thank you @kroegerama for his awesome work.
@kroegerama
Merge pull request#2from OpenAPITools/master
80ef77b
merge from OpenAPITools/openapi-generator master
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
@kroegerama
improve performance, hide debug info
4db0206
* add retrofit scalars converter for simple types
* only add HttpLoggingInterceptor in Debug builds
* initialize OkHttpClient lazy on non-main thread (see https://www.zacsweers.dev/dagger-party-tricks-deferred-okhttp-init/)
@kroegerama
update okHttp version
735ed18
@kroegerama
add api version constant to RetrofitHolder
5cc0063
@kroegerama
add versionName to build.gradle
55b443d
jimschubert
jimschubert
reviewed
Nov 18, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 setCollectionType(additionalProperties.get(COLLECTION_TYPE).toString());
 }

 if (CollectionType.LIST.value.equals(collectionType)) {
@jimschubertjimschubertNov 14, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This should have a warning that we're modifying typeMappings at this point, because it'll beak user's ability to define type mappings when using generator defaults.
Comment threadmodules/openapi-generator/src/main/resources/kotlin-retrofit/api.mustache
import retrofit2.create
import retrofit2.http.*
import okhttp3.*
import retrofit2.http.Headers
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Is this necessary with the glob import above?
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 supportingFiles.add(new SupportingFile("proguard-rules.pro.mustache", "", "proguard-rules.pro"));
 supportingFiles.add(new SupportingFile("gitignore.mustache", "", ".gitignore"));

 supportingFiles.add(new SupportingFile("AndroidManifest.xml.mustache", "src/main", "AndroidManifest.xml"));
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Note: we may want to put Android options behind a CLI option. I scanned and didn't see any Android specific code in client files.
@jimschubert
jimschubert
commented
Nov 18, 2019

Copy link


Copy Markdown

Member
I agree that this would make more sense as a library option to the existing client generator. It seems that one of the major reasons it wasn't was because some keywords (like data) are set as reserved but disallowed for variables. Many of these aren't allowed elsewhere (can't recall exactly, but I think as Enums), so I think any special casing logic makes the most sense in the base class to benefit all Kotlin generators.
What might you need from the core team to get this into master? Maybe we could do it as a separate Beta generator and move to a library option later?
@wing328
wing328
commented
Dec 14, 2019

Copy link


Copy Markdown

Member
Here is another PR to add retrofit2 as a library option to the Kotlin client generator: #4518
@wing328
wing328
commented
Dec 22, 2019

Copy link


Copy Markdown

Member
#4518 has been merged into master.
Please give it a try. Closing this one for the time being.
@wing328wing328
closed this
Dec 22, 2019
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jimschubertjimschubertjimschubert left review comments
@wing328wing328wing328 left review comments

+1 more reviewer

@MatkovIvanMatkovIvanMatkovIvan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Client: KotlinClient: RetrofitEnhancement: New generator

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@kroegerama@wing328@snackycracky@jakoss@craigmurray1120@jimschubert@MatkovIvan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
New generator: kotlin-retrofit (lightweight, Android, coroutines) by kroegerama · Pull Request #3195 · OpenAPITools/openapi-generator · GitHub
Skip to content
New generator: kotlin-retrofit (lightweight, Android, coroutines) - #3195
Closed
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Closed
New generator: kotlin-retrofit (lightweight, Android, coroutines)#3195
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Conversation
@kroegerama
@kroegeramakroegerama
commented
Jun 21, 2019

Copy link


Copy Markdown

Contributor
See #3135
@kroegerama
add new generator: kotlin-retrofit
23d5d5e
@kroegerama
Merge branch 'master' ofhttps://github.com/OpenAPITools/openapi-gene…
67686e3
…rator into kotlin-retrofit
@kroegerama
improve generated code quality
9f49935
* KotlinRetrofitCodegen no more uses AbstractKotlinCodegen (has wrong reserved word list)
* support for default values for Api parameters
@kroegerama
add support for allOf (inheritance)
1e36f85
@kroegerama
Merge pull request#1from OpenAPITools/master
80aa5b4
merge latest master
@auto-labeler
auto-labelerBot
commented
Jun 21, 2019

Copy link


Copy Markdown

👍 Thanks for opening this issue!

🏷 I have applied any labels matching special text in your issue.
The team will review the labels and make any necessary changes.
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
@kroegerama thanks for contributing a new generator.
Have you considered incorporating the retrofit support via a --library option to the existing kotlin client generator?
The current java client generator supports many HTTP libraries via the --library option.
@wing328wing328
added this to the 4.0.3 milestone
Jun 24, 2019
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
cc Kotlin tech committe: @jimschubert (2017/09) , @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04)
@wing328wing328
modified the milestones:
4.0.3, 4.1.0Jul 9, 2019
MatkovIvan
MatkovIvan
reviewed
Jul 19, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java

import static org.openapitools.codegen.utils.StringUtils.*;

public class KotlinRetrofitCodegen extends DefaultCodegen implements CodegenConfig {
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I guess AbstractKotlinCodegen should be used here
@kroegeramakroegeramaJul 19, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Nope. AbstractKotlinCodegen has some wrong implementations. See my explanation here which is just one error I found in the abstract class.

We could merge the corrections in my Codegen into AbstractKotlinCodegen, though.
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Fix found error in AbstractKotlinCodegen is the right option. Plus, it can be done in separate PR to make contributions smaller -> get reviews easier -> merge faster.
BTW it's a nice contribution, looking forward to it 👍
@wing328wing328Jul 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Agreed with @MatkovIvan to fix the bug in AbstractKotlinCodegen instead.
@snackycracky
snackycracky
commented
Aug 4, 2019

Copy link


Copy Markdown

hey, thanks for the effort. Would be awesome if any kind of authenticationScheme is supported :)
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@jakoss
jakoss
commented
Sep 6, 2019

Copy link


Copy Markdown

Hey. Great idea you have there. Do you have any ETA on release? Maybe i could somehow help push this further?
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@craigmurray1120
craigmurray1120
commented
Sep 20, 2019

Copy link


Copy Markdown

Would love to see this generator gets merged into master.
Thank you @kroegerama for his awesome work.
@kroegerama
Merge pull request#2from OpenAPITools/master
80ef77b
merge from OpenAPITools/openapi-generator master
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
@kroegerama
improve performance, hide debug info
4db0206
* add retrofit scalars converter for simple types
* only add HttpLoggingInterceptor in Debug builds
* initialize OkHttpClient lazy on non-main thread (see https://www.zacsweers.dev/dagger-party-tricks-deferred-okhttp-init/)
@kroegerama
update okHttp version
735ed18
@kroegerama
add api version constant to RetrofitHolder
5cc0063
@kroegerama
add versionName to build.gradle
55b443d
jimschubert
jimschubert
reviewed
Nov 18, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 setCollectionType(additionalProperties.get(COLLECTION_TYPE).toString());
 }

 if (CollectionType.LIST.value.equals(collectionType)) {
@jimschubertjimschubertNov 14, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This should have a warning that we're modifying typeMappings at this point, because it'll beak user's ability to define type mappings when using generator defaults.
Comment threadmodules/openapi-generator/src/main/resources/kotlin-retrofit/api.mustache
import retrofit2.create
import retrofit2.http.*
import okhttp3.*
import retrofit2.http.Headers
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Is this necessary with the glob import above?
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 supportingFiles.add(new SupportingFile("proguard-rules.pro.mustache", "", "proguard-rules.pro"));
 supportingFiles.add(new SupportingFile("gitignore.mustache", "", ".gitignore"));

 supportingFiles.add(new SupportingFile("AndroidManifest.xml.mustache", "src/main", "AndroidManifest.xml"));
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Note: we may want to put Android options behind a CLI option. I scanned and didn't see any Android specific code in client files.
@jimschubert
jimschubert
commented
Nov 18, 2019

Copy link


Copy Markdown

Member
I agree that this would make more sense as a library option to the existing client generator. It seems that one of the major reasons it wasn't was because some keywords (like data) are set as reserved but disallowed for variables. Many of these aren't allowed elsewhere (can't recall exactly, but I think as Enums), so I think any special casing logic makes the most sense in the base class to benefit all Kotlin generators.
What might you need from the core team to get this into master? Maybe we could do it as a separate Beta generator and move to a library option later?
@wing328
wing328
commented
Dec 14, 2019

Copy link


Copy Markdown

Member
Here is another PR to add retrofit2 as a library option to the Kotlin client generator: #4518
@wing328
wing328
commented
Dec 22, 2019

Copy link


Copy Markdown

Member
#4518 has been merged into master.
Please give it a try. Closing this one for the time being.
@wing328wing328
closed this
Dec 22, 2019
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jimschubertjimschubertjimschubert left review comments
@wing328wing328wing328 left review comments

+1 more reviewer

@MatkovIvanMatkovIvanMatkovIvan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Client: KotlinClient: RetrofitEnhancement: New generator

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@kroegerama@wing328@snackycracky@jakoss@craigmurray1120@jimschubert@MatkovIvan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
New generator: kotlin-retrofit (lightweight, Android, coroutines) by kroegerama · Pull Request #3195 · OpenAPITools/openapi-generator · GitHub
Skip to content
New generator: kotlin-retrofit (lightweight, Android, coroutines) - #3195
Closed
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Closed
New generator: kotlin-retrofit (lightweight, Android, coroutines)#3195
kroegerama wants to merge 15 commits into
OpenAPITools:masterfrom 
kroegerama:kotlin-retrofit
Conversation
@kroegerama
@kroegeramakroegerama
commented
Jun 21, 2019

Copy link


Copy Markdown

Contributor
See #3135
@kroegerama
add new generator: kotlin-retrofit
23d5d5e
@kroegerama
Merge branch 'master' ofhttps://github.com/OpenAPITools/openapi-gene…
67686e3
…rator into kotlin-retrofit
@kroegerama
improve generated code quality
9f49935
* KotlinRetrofitCodegen no more uses AbstractKotlinCodegen (has wrong reserved word list)
* support for default values for Api parameters
@kroegerama
add support for allOf (inheritance)
1e36f85
@kroegerama
Merge pull request#1from OpenAPITools/master
80aa5b4
merge latest master
@auto-labeler
auto-labelerBot
commented
Jun 21, 2019

Copy link


Copy Markdown

👍 Thanks for opening this issue!

🏷 I have applied any labels matching special text in your issue.
The team will review the labels and make any necessary changes.
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
@kroegerama thanks for contributing a new generator.
Have you considered incorporating the retrofit support via a --library option to the existing kotlin client generator?
The current java client generator supports many HTTP libraries via the --library option.
@wing328wing328
added this to the 4.0.3 milestone
Jun 24, 2019
@wing328
wing328
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
cc Kotlin tech committe: @jimschubert (2017/09) , @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04)
@wing328wing328
modified the milestones:
4.0.3, 4.1.0Jul 9, 2019
MatkovIvan
MatkovIvan
reviewed
Jul 19, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java

import static org.openapitools.codegen.utils.StringUtils.*;

public class KotlinRetrofitCodegen extends DefaultCodegen implements CodegenConfig {
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I guess AbstractKotlinCodegen should be used here
@kroegeramakroegeramaJul 19, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Nope. AbstractKotlinCodegen has some wrong implementations. See my explanation here which is just one error I found in the abstract class.

We could merge the corrections in my Codegen into AbstractKotlinCodegen, though.
@MatkovIvanMatkovIvanJul 19, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Fix found error in AbstractKotlinCodegen is the right option. Plus, it can be done in separate PR to make contributions smaller -> get reviews easier -> merge faster.
BTW it's a nice contribution, looking forward to it 👍
@wing328wing328Jul 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Agreed with @MatkovIvan to fix the bug in AbstractKotlinCodegen instead.
@snackycracky
snackycracky
commented
Aug 4, 2019

Copy link


Copy Markdown

hey, thanks for the effort. Would be awesome if any kind of authenticationScheme is supported :)
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@jakoss
jakoss
commented
Sep 6, 2019

Copy link


Copy Markdown

Hey. Great idea you have there. Do you have any ETA on release? Maybe i could somehow help push this further?
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@craigmurray1120
craigmurray1120
commented
Sep 20, 2019

Copy link


Copy Markdown

Would love to see this generator gets merged into master.
Thank you @kroegerama for his awesome work.
@kroegerama
Merge pull request#2from OpenAPITools/master
80ef77b
merge from OpenAPITools/openapi-generator master
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
@kroegerama
improve performance, hide debug info
4db0206
* add retrofit scalars converter for simple types
* only add HttpLoggingInterceptor in Debug builds
* initialize OkHttpClient lazy on non-main thread (see https://www.zacsweers.dev/dagger-party-tricks-deferred-okhttp-init/)
@kroegerama
update okHttp version
735ed18
@kroegerama
add api version constant to RetrofitHolder
5cc0063
@kroegerama
add versionName to build.gradle
55b443d
jimschubert
jimschubert
reviewed
Nov 18, 2019
View reviewed changes
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 setCollectionType(additionalProperties.get(COLLECTION_TYPE).toString());
 }

 if (CollectionType.LIST.value.equals(collectionType)) {
@jimschubertjimschubertNov 14, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This should have a warning that we're modifying typeMappings at this point, because it'll beak user's ability to define type mappings when using generator defaults.
Comment threadmodules/openapi-generator/src/main/resources/kotlin-retrofit/api.mustache
import retrofit2.create
import retrofit2.http.*
import okhttp3.*
import retrofit2.http.Headers
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Is this necessary with the glob import above?
Comment thread...penapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinRetrofitCodegen.java
 supportingFiles.add(new SupportingFile("proguard-rules.pro.mustache", "", "proguard-rules.pro"));
 supportingFiles.add(new SupportingFile("gitignore.mustache", "", ".gitignore"));

 supportingFiles.add(new SupportingFile("AndroidManifest.xml.mustache", "src/main", "AndroidManifest.xml"));
@jimschubertjimschubertNov 18, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Note: we may want to put Android options behind a CLI option. I scanned and didn't see any Android specific code in client files.
@jimschubert
jimschubert
commented
Nov 18, 2019

Copy link


Copy Markdown

Member
I agree that this would make more sense as a library option to the existing client generator. It seems that one of the major reasons it wasn't was because some keywords (like data) are set as reserved but disallowed for variables. Many of these aren't allowed elsewhere (can't recall exactly, but I think as Enums), so I think any special casing logic makes the most sense in the base class to benefit all Kotlin generators.
What might you need from the core team to get this into master? Maybe we could do it as a separate Beta generator and move to a library option later?
@wing328
wing328
commented
Dec 14, 2019

Copy link


Copy Markdown

Member
Here is another PR to add retrofit2 as a library option to the Kotlin client generator: #4518
@wing328
wing328
commented
Dec 22, 2019

Copy link


Copy Markdown

Member
#4518 has been merged into master.
Please give it a try. Closing this one for the time being.
@wing328wing328
closed this
Dec 22, 2019
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jimschubertjimschubertjimschubert left review comments
@wing328wing328wing328 left review comments

+1 more reviewer

@MatkovIvanMatkovIvanMatkovIvan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Client: KotlinClient: RetrofitEnhancement: New generator

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@kroegerama@wing328@snackycracky@jakoss@craigmurray1120@jimschubert@MatkovIvan