Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kotlin Coroutines Support #29

Merged
merged 2 commits into from
Jan 6, 2020
Merged

Kotlin Coroutines Support #29

merged 2 commits into from
Jan 6, 2020

Conversation

cortinico
Copy link
Collaborator

@cortinico cortinico commented May 24, 2019

This is an initial PR for supporting another template to generate Kotlin-coroutines code.
I'd love to get some early feedback from @macisamuele before I continue working on this to decide how we want to setup the project.

I'm not a big fan of copying all the .mustache files over, so we should probably re-organize them in subfolders?

What's missing:

  • Tests
  • Updated the readme
  • Add a kotlin-coroutines to the samples

@cortinico cortinico self-assigned this May 24, 2019
@macisamuele
Copy link
Collaborator

@cortinico we already discussed about the PR in person, but it would be better to keep a trace over here as well.

I do like the idea of having a new "template" that allows generation of coroutine based code.
Duplicating code and templates as provided in this PR could be a possibility but I do expect higher maintenance costs.

What if we do have a generic KotlinGenerator that will be extended by two independent generators (ie: KotlinRxJavaGenerator and KotlinCoroutineGenerator)?
By doing so we can ensure that all the kotlin specific details are defined once and reused.

The needed additions would be:

  • having a method that given the output type generates the appropriate string (ie. Type -> Single<Type>|Completable|Defer<Type>|Type according to the case)
  • templating the method modifiers to allow injection of suspend keyword in the API methods generation

NOTE: Doing something like this will then require a major release as the original currently existing template would not be usable anymore

@cortinico cortinico added this to the 1.2.0 milestone Jun 13, 2019
@cortinico cortinico modified the milestones: 1.2.0, 1.3.0 Jul 30, 2019
@cortinico
Copy link
Collaborator Author

After some discussion with @macisamuele we feel more comfortable pushing this to a future release

@chathudan
Copy link

@cortinico are you planning to release this sooner?

@cortinico
Copy link
Collaborator Author

@cortinico are you planning to release this sooner?

I'm currently looking for feedback on this. I would be interested in understanding if you're planning to use this modified template.
Adding coroutines support is quite straightforward, but it's yet another template that we have to maintain. Having active users that are the template is a valid reason to spend effort on it.

Regarding the timeline, ideally we can manage to have this merged and have a new release by the end of November.

@StepanMynarik
Copy link

I would love to use this, but I need it ASAP. Currently I'm using Swagger Codegen which has Kotlin coroutine support already.
I can't, however, force it to use ThreeTenABP LocalDate type instead of string for date fields. And it seems it will not be resolved in the foreseeable future :(

Is there a way to generate code with this pull request? I don't know how to download entire repository, patched with a specific PR out of GitHub. Also this codegen is used through Gradle I noticed. That makes it even more difficult for me then. At least a believe so. I'm new to Android Studio, Gradle and all, sorry.

Thanks

@cortinico cortinico marked this pull request as ready for review January 2, 2020 17:13
@cortinico cortinico requested a review from redwarp January 2, 2020 17:13
@cortinico cortinico added the enhancement New feature or request label Jan 2, 2020
@cortinico
Copy link
Collaborator Author

Hi all, please find a production-ready version of the coroutines support. Other than @macisamuele 's feedback I'd love to get an ack from either @redwarp or @filipemp.

I've took a different approach from the first commit. We now don't need to clone the entire .mustache file hierarchy.

@cortinico are you planning to release this sooner?

This will be in the upcoming release @chathudan, unless we find a major blocker during the review.

I would love to use this, but I need it ASAP.

@StevenKek apologize for the delay but I took a 2 months sabbatical and I haven't really checked my Github notifications :/ Ideally we're going to ship this soon and you'll have it bundled in the upcoming release.

Is there a way to generate code with this pull request?

You could checkout the fork and install the gradle plugin inside your maven local. It's doable but a bit complicated to setup. If you're not in a hurry, I advice you wait for the upcoming release.

Copy link
Collaborator

@macisamuele macisamuele left a comment

Choose a reason for hiding this comment

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

Very nice approach to the problem.
Reduce template duplication while preserving consistency and expandability!

🚢

@cortinico
Copy link
Collaborator Author

Very nice approach to the problem.
Reduce template duplication while preserving consistency and expandability!

Awesome! Thanks for the approval.
Let's wait for an ack from either @redwarp or @filipemp

@cortinico cortinico merged commit 9d9edce into master Jan 6, 2020
@cortinico cortinico deleted the coroutines-support branch January 6, 2020 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants