Skip to content
This repository has been archived by the owner on Feb 8, 2022. It is now read-only.

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

Closed
wants to merge 2 commits into from

Conversation

erikerlandson
Copy link

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
Copy link
Author

cc @SethTisue @johnynek

build.sbt Show resolved Hide resolved
case _ => false
}

lazy val scalaCheckVersion = "1.14.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

pretty sure this is binary incompat with 1.13, which breaks mima on the laws.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not following how a change to testing code breaks the package api compatability

build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
core/src/main/scala/algebra/ring/Additive.scala Outdated Show resolved Hide resolved
core/src/main/scala/algebra/ring/Additive.scala Outdated Show resolved Hide resolved
core/src/main/scala/algebra/ring/Additive.scala Outdated Show resolved Hide resolved
core/src/main/scala/algebra/ring/Multiplicative.scala Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Nov 28, 2018

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%) ⬇️
...e/src/main/scala/algebra/ring/Multiplicative.scala 49.15% <42.85%> (-1.8%) ⬇️
core/src/main/scala/algebra/ring/Additive.scala 40.67% <42.85%> (-2.18%) ⬇️

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
Copy link
Author

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

@SethTisue
Copy link
Member

tpolecat/tut#238 seems to indicate otherwise?

@erikerlandson
Copy link
Author

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

erikerlandson added a commit to erikerlandson/sbt-microsites that referenced this pull request Nov 28, 2018
typelevel algebra wants a version that can build against scala 2.13.0-M5 (see typelevel/algebra#221)
juanpedromoreno pushed a commit to 47degrees/sbt-microsites that referenced this pull request Nov 29, 2018
typelevel algebra wants a version that can build against scala 2.13.0-M5 (see typelevel/algebra#221)
@erikerlandson
Copy link
Author

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

@erikerlandson
Copy link
Author

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
Copy link
Author

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
Copy link
Author

@johnynek we are green

@erikerlandson
Copy link
Author

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
Copy link
Author

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
Copy link
Author

bump

Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

It would be a shame to lose the optimization of combineAll

@@ -7,7 +7,6 @@ import scala.annotation.tailrec
trait AdditiveSemigroup[@sp(Int, Long, Float, Double) A] extends Any with Serializable {
def additive: Semigroup[A] = new Semigroup[A] {
def combine(x: A, y: A): A = plus(x, y)
override def combineAllOption(as: TraversableOnce[A]): Option[A] = trySum(as)
Copy link
Contributor

Choose a reason for hiding this comment

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

did kernel remove combineAllOption? Why can't we override this?

@johnynek
Copy link
Contributor

johnynek commented Jan 8, 2019

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

@erikerlandson
Copy link
Author

@johnynek TraversableOnce does not exist in 2.13

@erikerlandson
Copy link
Author

This current draft is based on: #219 (comment)

But we can rework it

@johnynek
Copy link
Contributor

johnynek commented Jan 9, 2019

@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
Copy link
Author

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

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

@erikerlandson
Copy link
Author

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

@kailuowang
Copy link
Contributor

+1 on moving it into cats

@erikerlandson
Copy link
Author

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

@erikerlandson
Copy link
Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants