-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Make the value of the class private #2614
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2614 +/- ##
==========================================
- Coverage 95.17% 95.15% -0.03%
==========================================
Files 361 361
Lines 6658 6660 +2
Branches 303 303
==========================================
Hits 6337 6337
- Misses 321 323 +2
Continue to review full report at Codecov.
|
Thanks! Would you also add usages of the syntax to https://github.com/typelevel/cats/blob/master/binCompatTest/src/main/scala/catsBC/MimaExceptions.scala This will make sure that such usages are still binary compatible. |
once this approach is approved, I'd like to update all value classes fields to private in Cats |
@kailuowang Added. |
I think I've proven to myself that things still work as expected, so this should be ready for actual review. |
Thanks @coltfred |
@rossabaker @ceedubs @LukaJCB what do you think of this? |
This looks very reasonable, If everything still works as is then I'm happy to change this for all the extensions. Are we going to do an 1.5.0-RC1? We could make sure everything still works with that release :) |
@LukaJCB yes I think we should do a RC1 |
I'm more than happy to give a 1.5.0-RC1 a try today or this weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All binary exceptions make me nervous, but I think this one is justified. External calls to .value
would be silly, and the classes are final so nobody downstream is making a BiggerAndBetterOps class that needs it.
This makes the
.value
of two of theOps
classes private, since they shouldn't ever be referenced and they collide with Scalatest.Fixes #2514 and #2613