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

Add NonEmptyMap #2141

Merged
merged 16 commits into from
Mar 16, 2018
Merged

Add NonEmptyMap #2141

merged 16 commits into from
Mar 16, 2018

Conversation

LukaJCB
Copy link
Member

@LukaJCB LukaJCB commented Jan 8, 2018

This is basically just a copy-paste from https://github.com/LukaJCB/cats-maps-sets, I still need to add some more scaladoc and maybe a test here or there, but if someone has some early feedback I'd love to hear!

@LukaJCB LukaJCB mentioned this pull request Jan 8, 2018
@codecov-io
Copy link

codecov-io commented Jan 8, 2018

Codecov Report

Merging #2141 into master will increase coverage by 0.04%.
The diff coverage is 96.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2141      +/-   ##
==========================================
+ Coverage   94.76%   94.81%   +0.04%     
==========================================
  Files         331      332       +1     
  Lines        5659     5742      +83     
  Branches      208      212       +4     
==========================================
+ Hits         5363     5444      +81     
- Misses        296      298       +2
Impacted Files Coverage Δ
core/src/main/scala/cats/data/package.scala 87.5% <ø> (ø) ⬆️
core/src/main/scala/cats/data/NonEmptyList.scala 100% <100%> (ø) ⬆️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala 100% <100%> (ø) ⬆️
...ore/src/main/scala/cats/data/NonEmptyMapImpl.scala 95.94% <95.94%> (ø)
core/src/main/scala/cats/data/Kleisli.scala 97.93% <0%> (+0.04%) ⬆️
core/src/main/scala/cats/instances/sortedMap.scala 96.87% <0%> (+1.56%) ⬆️

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 3503cea...60327b1. Read the comment docs.

@jtjeferreira
Copy link
Contributor

jtjeferreira commented Jan 12, 2018

should cats.data.NonEmptyList#groupBy return an NonEmptyMap?

@LukaJCB
Copy link
Member Author

LukaJCB commented Jan 12, 2018

@jtjeferreira that's an excellent idea in my opinion, however it might not be feasible to change it though due to backwards compatibility.

@jtjeferreira
Copy link
Contributor

maybe add a new method groupByNem


import scala.collection.immutable._

final class NonEmptyMap[K, A] private(val value: SortedMap[K, A]) extends AnyVal {
Copy link
Contributor

Choose a reason for hiding this comment

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

does it have to be a SortedMap?

Copy link
Member Author

Choose a reason for hiding this comment

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

What would you want it to be?

Copy link
Contributor

Choose a reason for hiding this comment

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

just immutable.Map[K,A]?
or maybe a better name for the class then NonEmptySortedMap?

@LukaJCB LukaJCB added this to the 1.1 milestone Jan 15, 2018
@kailuowang
Copy link
Contributor

I will find some time Friday.

@LukaJCB
Copy link
Member Author

LukaJCB commented Jan 25, 2018

Excellent, thank you @kailuowang! No rush at all :)

@ceedubs
Copy link
Contributor

ceedubs commented Jan 25, 2018

@LukaJCB @kailuowang sorry, I'm unlikely to find time for any reviews until Feb 4th or so.

@LukaJCB
Copy link
Member Author

LukaJCB commented Jan 25, 2018

No worries at all @ceedubs! No need to rush :)

* res0: scala.collection.immutable.SortedSet[Int] = Set(1, 2)
* }}}
*/
def keys: SortedSet[K] = value.keySet
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be a non empty set

@LukaJCB LukaJCB requested a review from johnynek January 26, 2018 14:30
@@ -0,0 +1,343 @@
/*
* Copyright (c) 2018 Luka Jacobowitz
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we add a copyright header to all files in a separate PR (I am thinking maybe we can use sbt-header)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like a good idea!

Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we use copyright maintainers like the website or each file copyright goes to individual creators?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copyright maintainers seems best to me, I just copy and pasted this one from my own project 😄

kailuowang
kailuowang previously approved these changes Jan 29, 2018
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

LGTM

/**
* Adds a key-value pair to this map, returning a new `NonEmptyMap`.
* */
def +(ka: (K, A)): NonEmptyMap[K, A] = new NonEmptyMap(value + ka)
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to get rid of this operator because of the super annoying any2StringAdd :(

@@ -317,7 +322,7 @@ private[data] sealed abstract class NonEmptyMapInstances {
implicit def catsDataShowForNonEmptyMap[K: Show, A: Show]: Show[NonEmptyMap[K, A]] =
Show.show[NonEmptyMap[K, A]](_.show)

implicit def catsDataBandForNonEmptyMap[K, A]: Band[NonEmptyMap[K, A]] = new Band[NonEmptyMap[K, A]] {
implicit def catsDataSemilatticeForNonEmptyMap[K, A]: Semilattice[NonEmptyMap[K, A]] = new Semilattice[NonEmptyMap[K, A]] {
Copy link
Contributor

Choose a reason for hiding this comment

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

you changed it to semilattice here but not the law testing? I am not sure if Map combine is commutative.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right, yeah that was dumb, I'll check that one later :)

implicit def catsNonEmptyMapOps[K, A](value: Type[K, A]): NonEmptyMapOps[K, A] =
new NonEmptyMapOps(value)


Copy link
Member

Choose a reason for hiding this comment

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

Nitpicky, I know, but there are a lot of these double empty lines around.

kailuowang
kailuowang previously approved these changes Feb 5, 2018
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

noice.

Copy link
Contributor

@durban durban left a comment

Choose a reason for hiding this comment

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

I've left some comments.

@@ -11,6 +11,9 @@ package object data {
def NonEmptyStream[A](head: A, tail: A*): NonEmptyStream[A] =
OneAnd(head, tail.toStream)

type NonEmptyMap[K, +A] = NonEmptyMapImpl.Type[K, A]
val NonEmptyMap = NonEmptyMapImpl
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the inferred type here? I'd guess NonEmptyMapImpl.type, but that cannot be, since it's private[data] ...

Copy link
Member Author

Choose a reason for hiding this comment

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

It is its own type aka, cats.data.NonEmptyMap.type :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, sure ... but that's not what I asked. The inferred type seems to be this:

private[this] val NonEmptyMap: cats.data.NonEmptyMapImpl.type = NonEmptyMapImpl;
<stable> <accessor> def NonEmptyMap: cats.data.NonEmptyMapImpl.type = `package`.this.NonEmptyMap;

Which means that we have a private[data] type on a public API.

def one[K: Order, A](k: K, a: A): NonEmptyMap[K, A] =
create(SortedMap((k, a))(Order[K].toOrdering))

implicit def catsNonEmptyMapOps[K, A](value: Type[K, A]): NonEmptyMapOps[K, A] =
Copy link
Contributor

Choose a reason for hiding this comment

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

This is supposedly a public method, but its return type is private[data].

@@ -11,6 +11,9 @@ package object data {
def NonEmptyStream[A](head: A, tail: A*): NonEmptyStream[A] =
OneAnd(head, tail.toStream)

type NonEmptyMap[K, +A] = NonEmptyMapImpl.Type[K, A]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be called NonEmptySortedMap? There might be in the future a hashtable-based non-empty map ...


}

private[data] sealed class NonEmptyMapOps[K, A](val value: NonEmptyMapImpl.Type[K, A]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe private val value?

/**
* Returns a `SortedSet` containing all the keys of this map.
*/
def keys: SortedSet[K] = toSortedMap.keySet
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be NonEmpty(Sorted)Set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, as mentioned before here :) #2141 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

2141 merged :)

*/
def head: (K, A) = toSortedMap.head
/**
* Returns the first key-value pair of this map.
Copy link
Contributor

Choose a reason for hiding this comment

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

first -> last



/**
* Apply `f` to the "initial element" of this map and lazily combine it
Copy link
Contributor

Choose a reason for hiding this comment

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

What does lazily mean in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

A mistake 😄

package cats
package data

trait Newtype2 { self =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems kind of strange. Is this the same as the Newtype in the Set PR?

Copy link
Member

Choose a reason for hiding this comment

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

This one has two type parameters.


}

sealed class NonEmptyMapOps[K, A](val value: NonEmptyMap[K, A]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe private val value?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are just the Ops, what do we gain from making the value public? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing, that's why I suggested making it private :-)
In fact, I think it shouldn't be a val at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah, I got it switched around in my head 😄
Currently all of our Ops Syntax classes look this way and consistency is good the way I see it.
What would it be if not a val?

Copy link
Member

Choose a reason for hiding this comment

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

Adding val makes it a member on the class, omitting it, it's only in the constructor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, yeah that makes sense, I wonder why that's the convention for syntax classes in cats, maybe someone else can chime in 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@LukaJCB you have to make it public (ie val) if it's a value class, which is why you see it on other syntax classes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I remember correctly the reason it must be public is 2.10. For 2.11 and 2.12 private would work as well.

else throw new IllegalArgumentException("Cannot create NonEmptyMap from empty map")

def apply[K: Order, A](head: (K, A), tail: SortedMap[K, A]): NonEmptyMap[K, A] =
create(SortedMap(head)(Order[K].toOrdering) ++ tail)
Copy link
Contributor

Choose a reason for hiding this comment

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

This and I think some other methods don't follow the convention that we kind of settled on in the early days of Cats here. Admittedly, that never got added to the guidelines, and I'm not sure whether we have been consistent about that in other places.

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally don't have a strong opinion on this and didn't know we had such a convention 😄, you're right we should maybe reopen discussion and add it to the guideline, but for now I'll just change it, so we can avoid calling apply on the companion 👍 Thanks!

@LukaJCB
Copy link
Member Author

LukaJCB commented Mar 16, 2018

@ceedubs everything addressed? :)

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

@kailuowang kailuowang merged commit 55e0902 into typelevel:master Mar 16, 2018
@LukaJCB LukaJCB deleted the add-nonemptymap branch March 16, 2018 14:31
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.

10 participants