search

twitter / finatra

Fast, testable, Scala services built on TwitterServer and Finagle
https://twitter.github.io/finatra/
Apache License 2.0
2.27k stars 406 forks source link

inject-core: Cross-build for Scala 2.13 #530

Closed chrisbenincasa closed 4 years ago

chrisbenincasa commented 4 years ago

Depends on #529

Problem

inject-core is not cross-built for Scala 2.13.

Solution

Update inject-core module to cross-build for Scala 2.13 using new SBT settings. Adds in shim for old code no longer supported by Scalaguice to generate TypeLiterals from Manifests; it was simpler to do this than change the context bound everywhere (which led to a host of other compatibility issues).

Result

inject-core is cross-built for Scala 2.13.

chrisbenincasa commented 4 years ago

Note: we'll have to replace all of the uses of Scalaguice's typeLiteral as a part of this... but that might be less invasive to do this to keep existing behavior than changing the type signature of things like MessageBodyManager or DefaultMessageBodyReader (which could prove to be impossible anyway because eventually the impls of those things drill down to the Jackson Scala module, which still requires Manifests).

chrisbenincasa commented 4 years ago

@yufangong we will probably have another issue with dependencies here, since this PR had to bump nscala-time

chrisbenincasa commented 4 years ago

@yufangong if it's not too much trouble, I think it will be easier to go one by one - that way it'll be simpler for me incorporate any feedback into the correct commit without having to do some more complicated git to amend changes into a commit for a particular module that may be further up the chain.

chrisbenincasa commented 4 years ago

Or another idea.... if you want to make the merging / rebasing process simpler, we could do the reviews on each individual PR, mark the PR that we want to merge (that contains multiple commits) and once all of the PRs are approved and ready, I can rebase up the chain and you can just merge the final PR with N commits. Let me know which you think works best.

codecov[bot] commented 4 years ago

Codecov Report

Merging #530 into develop will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #530   +/-   ##
========================================
  Coverage    90.75%   90.75%           
========================================
  Files          267      267           
  Lines         4779     4779           
  Branches       302      301    -1     
========================================
  Hits          4337     4337           
  Misses         442      442

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 5fc7b3b...b79ea13. Read the comment docs.

yufangong commented 4 years ago

this is merged at https://github.com/twitter/finatra/commit/abb72f491b9fbe1b09e892dd322e5029c28f18b4. Thanks!!!

yufangong commented 4 years ago

@yufangong if it's not too much trouble, I think it will be easier to go one by one - that way it'll be simpler for me incorporate any feedback into the correct commit without having to do some more complicated git to amend changes into a commit for a particular module that may be further up the chain.

It's not, one by one works for me. We can also work on this incrementally, as the next level in the dependency graph is utils. ports, modules based on https://github.com/twitter/finatra/issues/522, if this is still accurate, we can start to rebase and merge in these three, I've left some comments on the three PRs. Thanks!