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

Updated coroutines to 0.30.0 and removed (most of) deprecated code #83

Merged
merged 8 commits into from
Oct 11, 2018

Conversation

gmariotti
Copy link
Contributor

@gmariotti gmariotti commented Oct 6, 2018

Next steps:

  • Adjust documentation for coroutines module. (how should I update the examples in index.adoc?)
  • Remove deprecated ArrayChannel in ReceiveChannelHandler.
  • Make project depend on Kotlin 1.3.x and support the latest coroutines version.

@vietj
Copy link
Contributor

vietj commented Oct 7, 2018

can you make sure that the stdlib used by coroutines is the same than the globally declared stdlib ?

pom.xml Show resolved Hide resolved
@vietj
Copy link
Contributor

vietj commented Oct 7, 2018

what is the interest to implement CoroutineScope for the CoroutineVerticle ?

@gmariotti
Copy link
Contributor Author

what is the interest to implement CoroutineScope for the CoroutineVerticle ?

Not an expert here, but my understanding is that CoroutineScope should be implemented by entities that have a defined lifecycle, like a Verticle, and that will start child coroutines, making each coroutine bounded to the parent scope by default, potentially avoiding memory leaks. @elizarov had an amazing talk at KotlinConf this year and he can definitely explain it better than me.

@vietj
Copy link
Contributor

vietj commented Oct 7, 2018

@gmariotti makes sense, I think this should be documented in the coroutine verticle and in the general documentation

@gmariotti
Copy link
Contributor Author

@vietj Updated the documentation of CoroutineVerticle. I was thinking of opening a new PR, after the one for ReceiveChannelHandler, to update the documentation for vertx-lang-kotlin-coroutines, is that ok? Also, is there any maven command to copy the examples to index.adoc or do they need to be manually copied?

@vietj
Copy link
Contributor

vietj commented Oct 7, 2018

I think if we document this, it should be in this PR.

for the docs : I think the current doc is wrong, it should contain asciidoc references that will copy the code as far as I remember, like explained in this blog : http://mrhaki.blogspot.com/2014/04/awesome-asciidoc-include-partial-parts.html . I think we need first a PR that fixes that.

@gmariotti
Copy link
Contributor Author

@vietj opened #84, but I'll need some help to better understand how the docs should be generated.

@vietj
Copy link
Contributor

vietj commented Oct 7, 2018

I'm going to take care of that for you

@vietj
Copy link
Contributor

vietj commented Oct 7, 2018

@gmariotti please use https://github.com/vert-x3/vertx-lang-kotlin/tree/gmariotti-asciidoc which is based on this PR, it provides the necessary hooks to use inclusion and preview (documented in README.adoc)

@gmariotti
Copy link
Contributor Author

@vietj should we merge your branch first and then I'll rebase this branch and adjust the documentation?

@vietj
Copy link
Contributor

vietj commented Oct 7, 2018

is the branch ready to be merged ? if yes, we can merge it as it serves a single purpose

@gmariotti
Copy link
Contributor Author

yes, I already took care of having all the examples in Example.kt

@gmariotti
Copy link
Contributor Author

I'll close #84, considering that there's already https://github.com/vert-x3/vertx-lang-kotlin/tree/gmariotti-asciidoc

@vietj
Copy link
Contributor

vietj commented Oct 9, 2018

what's the status of this PR ?

@gmariotti
Copy link
Contributor Author

@vietj sorry I didn't have time the last 2 days to update the documentation for the coroutines module. If it's fine for you, I'll try to do it before the end of the week

@vietj
Copy link
Contributor

vietj commented Oct 9, 2018

it's fine @gmariotti

@gmariotti
Copy link
Contributor Author

@vietj documentation updated, let me know if there's something I should fix.

@vietj vietj merged commit b9d0aca into vert-x3:master Oct 11, 2018
@vietj vietj added this to the 3.6.0 milestone Oct 11, 2018
@vietj
Copy link
Contributor

vietj commented Oct 11, 2018

thanks for the contribution @gmariotti

@vietj
Copy link
Contributor

vietj commented Oct 11, 2018

I think we need also to document the new Async coroutine method in 3.6.0

@gmariotti gmariotti deleted the coroutines branch October 11, 2018 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants