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

Reduce visibility of value member in the NonEmptyMap syntax #4559

Merged
merged 2 commits into from
Apr 8, 2024

Conversation

TonioGela
Copy link
Member

@TonioGela TonioGela commented Feb 14, 2024

Simple fix for this annoying behaviour:

//> using dep org.typelevel::cats-core::2.10.0

import cats.data.NonEmptyMap

val nem: NonEmptyMap[Int,Int] = NonEmptyMap.of(1 -> 1)
val alsoNem: NonEmptyMap[Int,Int] = nem.value.value.value.value

The fix passes the Mima check, but ofc it will become impossible to explicitly call .value on NonEmptyMap instances (but that's what we want, right?)
Thanks to @reardonj for the solution

@TonioGela TonioGela changed the title Reduce visibility of member in a syntax class Reduce visibility of value member in the NonEmptyMap syntax Feb 14, 2024
johnynek
johnynek previously approved these changes Feb 14, 2024
danicheg
danicheg previously approved these changes Feb 15, 2024
Copy link
Member

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

@TonioGela
Copy link
Member Author

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!

rossabaker
rossabaker previously approved these changes Feb 15, 2024
@TonioGela
Copy link
Member Author

TonioGela commented Feb 15, 2024

I noticed these for NonEmptyVector, NonEmptyList and NonEmptySeq

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?

@reardonj
Copy link
Contributor

I noticed these for NonEmptyVector, NonEmptyList and NonEmptySeq

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

@TonioGela
Copy link
Member Author

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?

@reardonj
Copy link
Contributor

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.

Copy link
Contributor

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

@TonioGela
Copy link
Member Author

Nice catch, thanks!

Thank you

I feel we should merge it – exposing "self" values in extension-like classes is hardly a good idea in general.

I've tried to spot them all, but we can't be 100% sure

@satorg
Copy link
Contributor

satorg commented Apr 8, 2024

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.

@satorg satorg merged commit fef949e into typelevel:main Apr 8, 2024
16 checks passed
@TonioGela TonioGela deleted the useless-public-member branch April 8, 2024 07:40
@TonioGela
Copy link
Member Author

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

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

Successfully merging this pull request may close these issues.

6 participants