-
-
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
Reduce visibility of value member in the NonEmptyMap
syntax
#4559
Conversation
NonEmptyMap
syntax
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.
It'd be nice to burn this out wherever possible at once. I recall the same approach is used for NonEmptySet
.
I'm on it! |
a9b2f73
I noticed these for object NonEmptyVector {
//...
class ZipNonEmptyVector[A](val value: NonEmptyVector[A]) extends Serializable
//...
} but they seem more like implementation details for some other syntax methods (and I'm not sure they're user-visible). Shall we hide these constructors, too? |
Like, private constructor even? That does looks like an implementation detail. class ZipNonEmptyVector[A] private[data](private[data] val value: NonEmptyVector[A]) extends Serializable |
I'm not sure I follow. You're suggesting me to either apply this method or you're thinking out loud if this is an implementation detail and we should leave it like so? |
That the class is an implementation detail and the whole thing could be hidden. Hmm; I guess package private can even go on a class. |
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.
Nice catch, thanks!
I feel we should merge it – exposing "self" values in extension-like classes is hardly a good idea in general.
Thank you
I've tried to spot them all, but we can't be 100% sure |
Let me go ahead and merge it not awaiting the second approval. It seems to be a straightforward but still quite important change, so if Mima is happy, I am more than happy too. |
Thanks @satorg |
Simple fix for this annoying behaviour:
The fix passes the Mima check, but ofc it will become impossible to explicitly call
.value
onNonEmptyMap
instances (but that's what we want, right?)Thanks to @reardonj for the solution