Enable MIMA in build, fix binary incompatibility

[sritchie]

Looks like we have another legit binary compatibility issue. This commit is the culprit: https://github.com/typelevel/algebra/commit/24fe85a71d58056d07c954f9a1a900159a053ea8

it renames Ring.fromDouble to defaultFromDouble.

[error]  * method defaultFromDouble(Double,algebra.ring.Ring,algebra.ring.MultiplicativeGroup)java.lang.Object in trait algebra.ring.RingFunctions is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("algebra.ring.RingFunctions.defaultFromDouble")
[info] core: found 1 potential binary incompatibilities while checking against org.typelevel:algebra_2.11:0.6.0
[error]  * method defaultFromDouble(Double,algebra.ring.Ring,algebra.ring.MultiplicativeGroup)java.lang.Object in trait algebra.ring.RingFunctions is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("algebra.ring.RingFunctions.defaultFromDouble")

[oscar-stripe]

Here is my solution here.

add something like:

object RingFunctions {
  implicit class AdditionalMethods[R[_]](val r: RingFunctions[R]) extends AnyVal {
    def  defaultFromDouble[T](d: Double)(implicit ....): T = ...
  }
}

then test that the old source syntax works on Ring.defaultFromDouble.

Alternatively, we could add this method to object Ring directly, which is safe.

What we should do is when we have a linear hierarchy use abstract class, which does not have this problem (that it is unsafe to add methods).

[codecov-io]

Current coverage is 73.64% (diff: 100%)

Merging #195 into master will decrease coverage by 0.34%

@@             master       #195   diff @@
==========================================
  Files            36         37     +1   
  Lines           769        774     +5   
  Methods         708        713     +5   
  Messages          0          0          
  Branches         61         57     -4   
==========================================
+ Hits            569        570     +1   
- Misses          200        204     +4   
  Partials          0          0          

Powered by Codecov. Last update 2c5bdad...36abbaa

[sritchie]

Okay, that fix seems to work locally and fix binary compatibility, @johnynek.

[oscar-stripe]

+1 from me. @non @tixxit @denisrosset et. al. any concerns with this approach?

[denisrosset]

Crap, binary compatibility is indeed the most important feature of algebra.

I like the idea of AdditionalMethods in general; here, as defaultFromDouble is merely a default implementation, I'd prefer putting it in a companion object (either Ring or a package-level DefaultMethods object) and avoid piling up implicits.

[sritchie-stripe]

@denisrosset I do prefer the companion object approach - the one thing we lose by moving it into Ring is Field.defaultFromDouble. I'm okay with that, but it may be an argument for the DefaultMethods object.

[johnynek]

We don't have to add a new class for each method, just one for each trait that we wind up wanting to change the methods for.

I also like that Field.defaultFromDouble will work. On Tue, Dec 6, 2016 at 05:40 Sam Ritchie notifications@github.com wrote:

@denisrosset https://github.com/denisrosset I do prefer the companion object approach - the one thing we lose by moving it into Ring is Field.defaultFromDouble. I'm okay with that, but it may be an argument for the DefaultMethods object.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/typelevel/algebra/pull/195#issuecomment-265151742, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEJdk1N6apAb25ngS-PqQwzJVVE7U9-ks5rFWXAgaJpZM4LEpe8 .

[sritchie-stripe]

@johnynek fair - and @denisrosset, we can go ahead and remove those and move the methods into the trait like you had originally done when we go to release 0.7.0. Thoughts?

[johnynek]

we could do 0.7.0 before spire or algebird release?

[non]

Sure, I think doing a 0.7.0 release before Spire and Algebird release makes sense.

[denisrosset]

+1

[sritchie]

Okay, great. I bumped the version, reverted the Ring changes and disabled mima in the build so we can do the release. After that let's turn it on again!

[non]

👍

I am away from the machine/GPG key I used to do releases. If someone else wants to do it please feel free. Otherwise I'll release Sunday when I am back.

[sritchie]

would someone mind kicking this? https://travis-ci.org/typelevel/algebra/jobs/181986215

This is a race condition in travis that I've seen before.

[johnynek]

Kicked it on the phone. If it doesn't work we'll look tomorrow

[sritchie]

Worked great. I

• Changed the way the default implicit gets built so that it exercises this constructor
• enabled MIMA and added a section with problems to the build, so we don't have to remember to mess with travis each time we want to allow a new binary incompatible version

[oscar-stripe]

👍

[sritchie]

@non or @tixxit, if one of y'all merge and release we can get this into algebird before the next release!