Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds version of mkString_ with no prefix/suffix, matching the std lib. #2709

Merged
merged 4 commits into from
Jan 31, 2019

Conversation

matthughes
Copy link
Contributor

Thank you for contributing to Cats!

This is a kind reminder to run sbt prePR and commit the changed files, if any, before submitting.

@matthughes
Copy link
Contributor Author

Fixes #2663 but as it stands, this is going to fail binary compatibility rules. Not sure how best to get around? I could change the name but I got no suggestions.

@kailuowang
Copy link
Contributor

kailuowang commented Jan 25, 2019

I am not sure if this is going to break binary compatibility. Right now the build breaks due to format issue, a sbt prePR will format the code and also check if it breaks binary compatibility.

@matthughes
Copy link
Contributor Author

So this does fail with binary incompatibility as you can see here: https://travis-ci.org/typelevel/cats/jobs/484371830.

I don't quite grok the FoldableOps/0/1 pattern but it looks like it has something to do with getting around bin incompatibility. If I this new method to FoldableOps0 instead of FoldableOps, MiMa is happy. But now I'm faced with how to implement mkString_. Previously, I was just doing a simple delegation to the prefix/d/suffix version, but that's no longer in scope. I could:

  1. copy the relevant implementation over into the other trait.
  2. call new FoldableOps(f).mkString_("", delim, "")

I'm find with either option but looking for input on how you'd like it.

@kailuowang
Copy link
Contributor

kailuowang commented Jan 25, 2019

@matthughes thanks!
The short answer is: yes, please move it FoldableOps0 and I'd perfer call new FoldableOps(f).mkString_("", delim, "")

The longer answer:
ah, it looks like adding an override to a method also breaks BC. Good to learn something new.
FoldableOps0 and FoldableOps1 were created recently to get around binary incompatibility. Mima is happy when you move the method to FoldableOps0 because that one is not officially released yet, it's released on 1.6.0-RC1, and mima doesn't check BC against RC releases. If we can make this one in 1.6.0 we should be good. Just noticed that your PR is against 1.6.x branch rather than master, so we are good on that as well.

@mpilquist
Copy link
Member

Note: adding an overload to a regular class doesn't break binary compat but doing so to a value class does. The existence of an overload changes the scheme used for generation of the synthetic method names in the companion class.

Consider these two classes, representing 2 different versions of the same class:

class Foo1(val x: Int) extends AnyVal {
  def foo(y: Int): Int = x + y
}

class Foo2(val x: Int) extends AnyVal {
  def foo(y: Int): Int = x + y
  def foo(y: Int, z: Int): Int = x + y + z
}

Foo1.class and Foo2.class are binary compatible (modulo the class name difference of Foo1 vs Foo2). However, Foo1$.class and Foo2$.class are not.

Foo1$.class has this method:

  public final int foo$extension(int, int);
    descriptor: (II)I
    flags: ACC_PUBLIC, ACC_FINAL

Foo2$.class has these methods:

  public final int foo$extension0(int, int);
    descriptor: (II)I
    flags: ACC_PUBLIC, ACC_FINAL
<snip>

  public final int foo$extension1(int, int, int);
    descriptor: (III)I
    flags: ACC_PUBLIC, ACC_FINAL

@kailuowang
Copy link
Contributor

@mpilquist ah, that makes sense, thanks for the detail! In hindsight, maybe the first one should've been made foo$extension0 rather than foo$extension. I'll see if I can find an existing scalac issue for this one.

@kailuowang
Copy link
Contributor

@matthughes I updated the CatsSuite which is missing the newly added syntax trait and caused the broken build. Hopefully this one will be ready to merge soon.

@codecov-io
Copy link

codecov-io commented Jan 31, 2019

Codecov Report

Merging #2709 into 1.6.x will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##            1.6.x    #2709      +/-   ##
==========================================
+ Coverage   95.12%   95.12%   +<.01%     
==========================================
  Files         365      365              
  Lines        6795     6796       +1     
  Branches      314      294      -20     
==========================================
+ Hits         6464     6465       +1     
  Misses        331      331
Impacted Files Coverage Δ
testkit/src/main/scala/cats/tests/CatsSuite.scala 70% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/foldable.scala 100% <100%> (ø) ⬆️

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 653b56f...68be218. Read the comment docs.

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

Successfully merging this pull request may close these issues.

6 participants