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

Remove useless Order constraint in NonEmptyMap constructors #2318

Closed
wants to merge 1 commit into from

Conversation

Kambius
Copy link

@Kambius Kambius commented Jul 8, 2018

Constructors which take SortedMap as a parameter don't need to take extra Order implicit as it is already there.

Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

If we have released with this it breaks binary compatibility. Otherwise looks good.

@Kambius
Copy link
Author

Kambius commented Jul 9, 2018

@johnynek yes, but it's not a big deal to fix client code with this changes. NonEmptyMap isn't used widely so maybe we could break binary compatibility? What other options we have if it's not the case?

@LukaJCB
Copy link
Member

LukaJCB commented Aug 1, 2018

The other option is to wait until 2.0 unfortunately :/

@larsrh
Copy link
Contributor

larsrh commented Sep 25, 2018

Couldn't the same hack apply here as elsewhere? Duplicate the methods and mark them as private[cats]?

@diesalbla
Copy link
Contributor

@LukaJCB Was this Pull Request applied on a different form? The cats 2.0 already came.

@diesalbla
Copy link
Contributor

Would it work to introduce a binary-compatible yet deprecated method, which used no implicits for the Order paramter, but which would generate the same bytecode-level, uncurrified method?

  def fromMap[K, A](as: SortedMap[K, A], k: Order[K]): Option[NonEmptyMap[K, A]] = 
    this.fromMap(as)

@joroKr21
Copy link
Member

Superseded by #3473

@larsrh larsrh closed this Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants