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

Make the value of the class private #2614

Merged
merged 3 commits into from
Nov 16, 2018
Merged

Conversation

coltfred
Copy link
Contributor

@coltfred coltfred commented Nov 15, 2018

This makes the .value of two of the Ops classes private, since they shouldn't ever be referenced and they collide with Scalatest.

Fixes #2514 and #2613

@codecov-io
Copy link

codecov-io commented Nov 15, 2018

Codecov Report

Merging #2614 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/nested.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/either.scala 99.26% <ø> (ø) ⬆️
...patTest/src/main/scala/catsBC/MimaExceptions.scala 0% <0%> (ø) ⬆️

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 564e492...e3df750. Read the comment docs.

@kailuowang
Copy link
Contributor

Thanks! Would you also add usages of the syntax to https://github.com/typelevel/cats/blob/master/binCompatTest/src/main/scala/catsBC/MimaExceptions.scala
something like
"blah".leftNec[Int]

This will make sure that such usages are still binary compatible.

@kailuowang
Copy link
Contributor

once this approach is approved, I'd like to update all value classes fields to private in Cats

@coltfred
Copy link
Contributor Author

@kailuowang Added.

@coltfred
Copy link
Contributor Author

I think I've proven to myself that things still work as expected, so this should be ready for actual review.

@kailuowang kailuowang added this to the 1.5 milestone Nov 15, 2018
@kailuowang
Copy link
Contributor

Thanks @coltfred

@kailuowang
Copy link
Contributor

@rossabaker @ceedubs @LukaJCB what do you think of this?
This is mostly to fix to the public field of value classes causing conflicts with other extensions. We had to make them public due to restrictions on scala 2.10 but we already dropped 2.10 support. If you all agree with this approach, I am going to create an issue to make all* value class fields private.

@kailuowang kailuowang requested review from ceedubs, LukaJCB and rossabaker and removed request for ceedubs November 15, 2018 22:20
@LukaJCB
Copy link
Member

LukaJCB commented Nov 16, 2018

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 :)

@kailuowang kailuowang modified the milestones: 1.5-RC0, 1.5.0-RC1 Nov 16, 2018
@kailuowang
Copy link
Contributor

@LukaJCB yes I think we should do a RC1

@coltfred
Copy link
Contributor Author

I'm more than happy to give a 1.5.0-RC1 a try today or this weekend.

Copy link
Member

@rossabaker rossabaker left a 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.

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.

6 participants