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

Fix NestedIdOps.value leaking to types of shapes F[G[A]] #2423

Merged
merged 2 commits into from
Sep 12, 2018
Merged

Fix NestedIdOps.value leaking to types of shapes F[G[A]] #2423

merged 2 commits into from
Sep 12, 2018

Conversation

zarthross
Copy link
Contributor

Made 'value' private in NestedIdOps
This prevents 'value' from leaking into types with shape F[G[A]].

I encountered this issue while using Scalatests OptionValues on things with a G[A] shape, so I had a Option[G[A]] and tried to use .value. It caused an ambiguous implicit match with NestedIdOps.

I didn't add a test, because I didn't know where it should go. Would SyntaxSuite.testNested be an appropriate place?

@zarthross
Copy link
Contributor Author

@kailuowang Should I have changed that to
private[syntax] val value: F[G[A]]
Instead?

If I'm reading this right, it avoids binary incompatibility?

@kailuowang
Copy link
Contributor

Sounds a good plan.

@codecov-io
Copy link

codecov-io commented Aug 25, 2018

Codecov Report

Merging #2423 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2423   +/-   ##
=======================================
  Coverage   95.27%   95.27%           
=======================================
  Files         351      351           
  Lines        6350     6350           
  Branches      274      274           
=======================================
  Hits         6050     6050           
  Misses        300      300
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/nested.scala 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 d37dcac...ce71612. Read the comment docs.

@zarthross
Copy link
Contributor Author

@kailuowang I've fixed the binary compatibility issue, mind removing the label?

Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

👍 LGTM.

Ideally we'd eventually want this to be changed back from package-private to private, wouldn't we? I'm wondering whether we should have a formal process for decluttering the noise that we add for binary compatibility (once we get to the point that we want to make a binary-incompatible release).

@kailuowang kailuowang merged commit cd3f659 into typelevel:master Sep 12, 2018
@kailuowang kailuowang added this to the 1.4 milestone Sep 12, 2018
@kailuowang kailuowang added the bug label Sep 12, 2018
ArturGajowy added a commit to ArturGajowy/rchain that referenced this pull request Jan 4, 2019
@ArturGajowy ArturGajowy mentioned this pull request Jan 4, 2019
5 tasks
ArturGajowy added a commit to ArturGajowy/rchain that referenced this pull request Jan 4, 2019
bors bot added a commit to rchain/rchain that referenced this pull request Jan 4, 2019
2066: Bump cats r=ArturGajowy a=ArturGajowy

The `.value` removals are probablye due to
typelevel/cats#2423



Co-authored-by: Artur Gajowy <artur.gajowy@gmail.com>
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.

7 participants