search

asynkron / protoactor-kotlin

Ultra-fast distributed cross-platform actor framework
http://proto.actor
Apache License 2.0
221 stars 25 forks source link

Upgrade gradle to 5.5.1 #50

Closed guenhter closed 5 years ago

guenhter commented 5 years ago
codecov[bot] commented 5 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@ccedb16). Click here to learn what that means. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #50   +/-   ##
=========================================
  Coverage          ?   31.67%           
  Complexity        ?      183           
=========================================
  Files             ?       50           
  Lines             ?     1746           
  Branches          ?      257           
=========================================
  Hits              ?      553           
  Misses            ?     1120           
  Partials          ?       73

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ccedb16...86fa60b. Read the comment docs.

james-cobb commented 5 years ago

@guenhter We have quite a few dependency versions (eg protobuf) repeated in multiple modules. Is it possible to extract the versions as a constant in the root build file so it's not repeated? And if there are any dependencies needed in all modules they could be declared in the root build.

I'll find some time to update our project that uses proto-actor to use this branch, and check if there are any problems updating versions.

guenhter commented 5 years ago

Good catch. I'll do that.

guenhter commented 5 years ago

I'll find some time to update our project that uses proto-actor to use this branch, and check if there are any problems updating versions.

I guess when a new release of this protoactor-kotlin is published (0.0.3) it will definitly break some users, because the current latest release 0.0.2 uses very outdated dependencies of e.g. coroutines when they were not yet stable. Now we use the stable version 1.x...

guenhter commented 5 years ago

We have quite a few dependency versions (eg protobuf) repeated in multiple modules. Is it possible to extract the versions as a constant in the root build file so it's not repeated? And if there are any dependencies needed in all modules they could be declared in the root build.

@james-cobb done.

james-cobb commented 5 years ago

Thanks for pulling out the versions as constants. Looks good.

I've never looked closely at all the build files and dependencies. But your work tidying all this up has got me thinking about it properly. So I'm afraid I've got another couple of questions now !!

I read up on the difference between api and implementation - is there good reason for the api declarations we have? Do they all need to be exposed to applications that use the library?

And a few dependencies weren't the very latest version - maybe a good reason, or in at least one case just a very recent (last few days) release:

nebula-bintray-plugin is on 7.3.0 - we depend on 6.0.6 slf4j is on 1.7.28 - we depend on 1.7.26 (very recent releases) kotlin logging is on 1.7.4 - we depend on 1.7.3

Otherwise I think the PR is good to merge.

I've used this branch in our main project, and the only changes needed were for the coroutine api changes - nothing else unexpected broke.

I did discover that there's a library API change introduced in the December 2018 commits that I can do a quick PR for before we go for a new release.

guenhter commented 5 years ago

Do they all need to be exposed to applications that use the library?

I guess most of them are correct as they are. API should be e.g. used, when a class of a dependency is used not only internally but is exposed. When all the classes of a dependency are just used internally, then implementation is the right one.

nebula-bintray-plugin is on 7.3.0 - we depend on 6.0.6

Lot of things have changed here. I guess this would be worth its own pull request...

slf4j is on 1.7.28 - we depend on 1.7.26 (very recent releases) kotlin logging is on 1.7.4 - we depend on 1.7.3

True, but such patch versions change every day. I'll update them next time when I check for dependency updates.

james-cobb commented 5 years ago

Thanks for the responses @guenhter - I think we should merge this PR. @orjan do you want to review, or should I merge?

james-cobb commented 5 years ago

(once we've merged this - I'm working on a PR to revert the DefaultDispatcher API to the way it was, and also hopefully remove use of GlobalScope that was introduced updating the coroutine APIs. If possible I'd like to do that before we release 0.0.3)

james-cobb commented 5 years ago

@guenhter You requested a review from Orjan - are you happy for this PR to be merged without his review?

guenhter commented 5 years ago

I have no problem with that, as long a second pair of eyes looked over it.

james-cobb commented 5 years ago

Well, may not be worth much, but I looked over it in detail ;)