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

Private val in value classes (Resolve TODO since 2.10 is dropped) #2692

Merged
merged 6 commits into from
Jan 17, 2019
Merged

Private val in value classes (Resolve TODO since 2.10 is dropped) #2692

merged 6 commits into from
Jan 17, 2019

Conversation

barambani
Copy link
Contributor

In relation to #2673 , this adds few more privates to constructor's parameters when extending AnyVal.
In the case of Either's CatchOnlyPartiallyApplied I added an exception to MiMa as there are good chances no one is really using dummy.

@barambani barambani changed the title Private val in value classes Private val in value classes (Resolve TODO since 2.10 is dropped) Jan 9, 2019
@kailuowang kailuowang added this to the 1.6 milestone Jan 9, 2019
@barambani
Copy link
Contributor Author

As a side note, I tried also working around the BC issues when doing the same in the Boilerplate generated syntaxes' Ops (that actually don't even extend AnyVal) but it's big work there. I will open a different PR if I find a reasonable way.

@kailuowang
Copy link
Contributor

@barambani thanks very much. No need to work around BC issue for this one. We decided in #2617 that we should just add mima exceptions. If you want to be really sure, you can add a special mima test like this one https://github.com/typelevel/cats/pull/2614/files#diff-978c9b9bcdbb84476d4105d6abb2c151L21

@kailuowang kailuowang added the bug label Jan 9, 2019
@barambani
Copy link
Contributor Author

I will try. The issue there is that, for the generated arity syntax functions, the MiMa exceptions needed are a lot, especially in 2.11.12. I will see if that's doable in a decent way. 👍

@codecov-io
Copy link

codecov-io commented Jan 9, 2019

Codecov Report

Merging #2692 into master will decrease coverage by 0.27%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2692      +/-   ##
==========================================
- Coverage   95.13%   94.85%   -0.28%     
==========================================
  Files         365      365              
  Lines        6758     6760       +2     
  Branches      289      288       -1     
==========================================
- Hits         6429     6412      -17     
- Misses        329      348      +19
Impacted Files Coverage Δ
...re/src/main/scala/cats/syntax/traverseFilter.scala 100% <ø> (ø) ⬆️
...laws/src/main/scala/cats/kernel/laws/package.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/foldable.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/reducible.scala 100% <ø> (ø) ⬆️
laws/src/main/scala/cats/laws/package.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/either.scala 86.76% <ø> (-12.5%) ⬇️
...patTest/src/main/scala/catsBC/MimaExceptions.scala 0% <0%> (ø) ⬆️
testkit/src/main/scala/cats/tests/CatsSuite.scala 33.33% <0%> (-36.67%) ⬇️
.../src/main/scala/cats/data/RepresentableStore.scala 83.33% <0%> (-4.17%) ⬇️
... and 17 more

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 17293ed...46c0834. Read the comment docs.

@barambani
Copy link
Contributor Author

barambani commented Jan 16, 2019

I think this ready to be reviewed. Unfortunately I cannot change the generated TupleXSemigroupalOps to value classes or the MimaExceptions test

(1.asRight[String], 2.asRight[String], 3.asRight[String]) mapN (_ + _ + _)

wouldn't pass.

UPDATE: changed the test to

(1.validNel[String], 2.validNel[String], 3.validNel[String]) mapN (_ + _ + _)

@barambani
Copy link
Contributor Author

Could the JVM 2.12's job be restarted ? Thanks

Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

Thanks so much!

@kailuowang
Copy link
Contributor

Quick question though, how come the changed test passed the mima test?

@barambani
Copy link
Contributor Author

Sorry, lack of details from me. I changed the test just to make it more representative of the real life use cases. The change is only from Either to Validated as it makes more sense to use mapN on the latter. The situation didn't change. The boilerplate generated syntax for Semigroupal cannot be changed or it will fail.

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

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

Successfully merging this pull request may close these issues.

4 participants