search

typelevel / algebra

Experimental project to lay out basic algebra type classes
https://typelevel.org/algebra/
Other
378 stars 69 forks source link

fixes #219 - publish for Scala 2.13.0-M5 #221

Closed erikerlandson closed 5 years ago

erikerlandson commented 5 years ago

Changes introduced in this PR:

  1. rev build deps to be compatible with scala 2.13.0-M5
  2. add scala 2.13.0-M5 build (also removes scala 2.10 build)
  3. remove references to TraversableOnce, replace with signatures for Iterable and Iterator

The dependencies on Semigroup, Monoid and friends from cats involved overriding methods like combineAllOption, which are still build using TraversableOnce. I removed the overrides and just let their default implementations apply.

erikerlandson commented 5 years ago

cc @SethTisue @johnynek

codecov-io commented 5 years ago

Codecov Report

Merging #221 into master will decrease coverage by 0.88%. The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
- Coverage    76.4%   75.52%   -0.89%     
==========================================
  Files          42       42              
  Lines         958      960       +2     
  Branches       68       45      -23     
==========================================
- Hits          732      725       -7     
- Misses        226      235       +9
Impacted Files Coverage Δ
core/src/main/scala/algebra/instances/map.scala 48% <ø> (-12%) :arrow_down:
...e/src/main/scala/algebra/ring/Multiplicative.scala 49.15% <42.85%> (-1.8%) :arrow_down:
core/src/main/scala/algebra/ring/Additive.scala 40.67% <42.85%> (-2.18%) :arrow_down:

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 f5f6a96...e01adf3. Read the comment docs.

erikerlandson commented 5 years ago

I think the remaining snag here is the tut plugin, which isn't built w/ 2.13

SethTisue commented 5 years ago

https://github.com/tpolecat/tut/issues/238 seems to indicate otherwise?

erikerlandson commented 5 years ago

the tut plugin dep is via sbt-microsites and that is using 0.6.6, which is a couple revs behind

erikerlandson commented 5 years ago

weird, I just pushed commit 0ff4081 above but CI isn't kicking off

erikerlandson commented 5 years ago

It's like github is mis-interpreting the sequence of events up there. I'm going to try squashing and force push to see if that helps.

erikerlandson commented 5 years ago

IIUC, I think I clobbered the test coverage of trySum when I removed the overrides for the Semigroup combineAll. Going to add property tests for trySum to restore coverage

erikerlandson commented 5 years ago

@johnynek we are green

erikerlandson commented 5 years ago

Just an observation - this change should be transparent to anybody using the package but it's a breaking change for anybody who is writing their own algebra typeclasses against it.

erikerlandson commented 5 years ago

Another possible approach to fixing this is to keep the TraversableOnce signature for improved binary compatability, but for 2.13 include a compatability shim like type TraversableOnce[A] IterableOnce[A]

Or conversely define everything as IterableOnce and use type IterableOnce[A] TraversableOnce[A] for pre 2.13

erikerlandson commented 5 years ago

bump

johnynek commented 5 years ago

So, if cats kernel is still using TraversableOnce, why do we have to change this one?

erikerlandson commented 5 years ago

@johnynek TraversableOnce does not exist in 2.13

erikerlandson commented 5 years ago

This current draft is based on: https://github.com/typelevel/algebra/issues/219#issuecomment-437602401

But we can rework it

johnynek commented 5 years ago

@erikerlandson but, what does cats have? did they just kill that method in cats.kernel, which we depend on...

we should really move this into the cats repo as cats-algebra. That will simplify this.

@kailuowang any concerns about that?

This repo predated cats, half of it became cats-kernel and we have talked about moving this in before as a new module.

erikerlandson commented 5 years ago

I was getting compile fails, but possibly TraversableOnce is just deprecated: https://github.com/scala/scala-collection-compat/issues/44 and maybe this is from -Xfatal-warnings ?

https://issues.scala-lang.org/browse/SI-8410

erikerlandson commented 5 years ago

@johnynek to answer your question, cats appears to still be making use of TraversableOnce

kailuowang commented 5 years ago

+1 on moving it into cats

erikerlandson commented 5 years ago

so there would be no algebra-0.8, but instead some cats-algebra release?

erikerlandson commented 5 years ago

I'm going to close this, since it sounds like the issue will be resolved by porting to a subproject of cats.