Add NonEmptySet #2143

LukaJCB · 2018-01-08T17:10:51Z

Pretty much the same situation as #2141

johnynek · 2018-01-08T17:18:47Z

core/src/main/scala/cats/data/NonEmptySet.scala

+
+import scala.collection.immutable._
+
+final class NonEmptySet[A] private (val set: SortedSet[A]) extends AnyVal {


AnyVal boxing can lead to a lot of reboxing when this is used in a generic context, which it would be in cats code since any of the typeclasses use it in generic context. I think we don't actually want AnyVal.

Good point! Man, we really need opaque types :(

johnynek · 2018-01-08T17:21:43Z

core/src/main/scala/cats/data/NonEmptySet.scala

+  def find(f: A ⇒ Boolean): Option[A] = set.find(f)
+  def collect[B](pf: PartialFunction[A, B]): Set[B] = set.collect(pf)
+  def filter(p: A ⇒ Boolean): SortedSet[A] = set.filter(p)
+  def filterNot(p: A ⇒ Boolean): SortedSet[A] = filter(t => !p(t))


I think we can also add concatMap[B](A => SortedSet[B]): SortedSet[B] and I think it would be useful. We can also do concatMap[B](A => NonEmptySet[B]): NonEmptySet[B]

Yeah, that's no doubt useful.

johnynek · 2018-01-08T17:24:42Z

core/src/main/scala/cats/data/NonEmptySet.scala

+
+import scala.collection.immutable._
+
+final class NonEmptySet[A] private (val set: SortedSet[A]) extends AnyVal {


nit: should this be NonEmptySortedSet? In the future we might want a Hash-based Set so just using Set can be ambiguous.

Hmm, yeah in theory this is true, but that's quite a long name, we could just have NonEmptySet and NonEmptyHashSet maybe?

johnynek · 2018-01-08T17:25:03Z

core/src/main/scala/cats/data/NonEmptySet.scala

+  def reduce[AA >: A](implicit S: Semigroup[AA]): AA =
+    S.combineAllOption(set).get
+
+  def toSet: SortedSet[A] = set


what about toSortedSet?

johnynek · 2018-01-08T17:25:43Z

core/src/main/scala/cats/data/NonEmptySet.scala

+  def of[A: Order](a: A, as: A*): NonEmptySet[A] =
+    new NonEmptySet(SortedSet(a)(Order[A].toOrdering) ++ SortedSet(as: _*)(Order[A].toOrdering) + a)
+  def apply[A: Order](head: A, tail: SortedSet[A]): NonEmptySet[A] = new NonEmptySet(SortedSet(head)(Order[A].toOrdering) ++ tail)
+  def one[A: Order](a: A): NonEmptySet[A] = new NonEmptySet(SortedSet(a)(Order[A].toOrdering))


do we need this one vs of? I also like singleton or something better.

I took the name from NonEmptyList.one, but I have no preference wrt naming.

I vote for consistent API

johnynek · 2018-01-08T17:27:28Z

core/src/main/scala/cats/data/NonEmptySet.scala

+  implicit def catsDataShowForNonEmptySet[A](implicit A: Show[A]): Show[NonEmptySet[A]] =
+    Show.show[NonEmptySet[A]](_.show)
+
+  implicit def catsDataBandForNonEmptySet[A]: Band[NonEmptySet[A]] = new Band[NonEmptySet[A]] {


this is a semilattice isn't it (it is commutative, no?) Am I missing something?

johnynek · 2018-01-08T17:29:15Z

tests/src/test/scala/cats/tests/NonEmptySetSuite.scala

+  checkAll("NonEmptySet[String]", BandTests[NonEmptySet[String]].band)
+  checkAll("NonEmptySet[String]", EqTests[NonEmptySet[String]].eqv)
+
+


can we test nes.forall { v => Order[Int].lteq(nes.head, v) }

codecov-io · 2018-01-08T19:50:53Z

Codecov Report

Merging #2143 into master will increase coverage by 0.1%.
The diff coverage is 97.4%.

@@            Coverage Diff            @@
##           master    #2143     +/-   ##
=========================================
+ Coverage   94.66%   94.76%   +0.1%     
=========================================
  Files         328      331      +3     
  Lines        5548     5659    +111     
  Branches      213      208      -5     
=========================================
+ Hits         5252     5363    +111     
  Misses        296      296

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/package.scala	`87.5% <ø> (ø)`	⬆️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala	`100% <100%> (+1.28%)`	⬆️
core/src/main/scala/cats/data/NonEmptySet.scala	`97.29% <97.29%> (ø)`
core/src/main/scala/cats/data/WriterT.scala	`91.37% <0%> (-1.76%)`	⬇️
.../src/main/scala/cats/syntax/applicativeError.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/syntax/option.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/syntax/TrySyntax.scala	`100% <0%> (ø)`
...c/main/scala/cats/free/ContravariantCoyoneda.scala	`100% <0%> (ø)`
core/src/main/scala/cats/syntax/either.scala	`99.16% <0%> (ø)`	⬆️
... and 3 more

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 d228e95...b709773. Read the comment docs.

LukaJCB · 2018-01-16T13:22:04Z

Maybe we should implement these with newtypes similar to what Alex did here: https://github.com/typelevel/cats-effect/pull/115/files#diff-c01f002766a28f49dfe20d6b1f0afd50R300

Thoughts?

kailuowang

LGTM

kailuowang · 2018-01-29T15:03:44Z

I am also fine with renaming it to NonEmptySortedSet, it's a long name, but not a very high cost to avoid potential confusion.

LukaJCB · 2018-01-31T17:48:09Z

core/src/main/scala/cats/data/NonEmptySet.scala

+
+final class NonEmptySet[A] private (val set: SortedSet[A]) {
+
+  private implicit def ordering: Ordering[A] = set.ordering


These should be vals

kailuowang · 2018-02-05T15:37:45Z

core/src/main/scala/cats/data/NonEmptySet.scala

+  private[cats] type Type[A] <: Base with Tag
+}
+
+private[data] object NonEmptySetImpl extends NonEmptySetInstances with Newtype {


shall we override the toString as well?

We can't really do that, since we can't actually add methods on the type, but only through implicit extension methods. This means that it will always call the toString of the underlying type, we can of course use show though.

andyscott · 2018-02-05T18:44:04Z

core/src/main/scala/cats/data/NonEmptySet.scala

+
+import scala.collection.immutable._
+
+trait Newtype { self =>


If it's staying public (or even package private) then let's move NewType to a new file.

LukaJCB · 2018-02-09T15:43:14Z

@johnynek Care to re-review? :)

ceedubs · 2018-02-11T19:23:15Z

core/src/main/scala/cats/data/NonEmptySet.scala

+
+
+  def of[A: Order](a: A, as: A*): NonEmptySet[A] =
+    create(SortedSet(a)(Order[A].toOrdering) ++ SortedSet(as: _*)(Order[A].toOrdering) + a)


Is this adding a to the set twice? (Initializing with SortedSet(a) and then calling + a.)

This is making two separate calls to toOrdering. If the ordering is needed twice, a slight optimization would be to store the result of toOrdering. Also, on a more minor note, the Order[A] instance is being summoned twice, so if the implicit instance is defined by a def this will result in unnecessary allocations.

Yeah, you're absolutely right, this is a really stupid mistake, not sure how it slipped through, probably refactored it one to many times 🙈

ceedubs · 2018-02-11T19:28:12Z

core/src/main/scala/cats/data/Newtype.scala

+package cats
+package data
+
+trait Newtype { self =>


Can you add some documentation that motivates this trait?

Yes, good idea :)

ceedubs · 2018-02-11T19:28:47Z

core/src/main/scala/cats/data/NonEmptySet.scala

@@ -0,0 +1,410 @@
+/*
+ * Copyright (c) 2018 Luka Jacobowitz


Is there a reason that this file has this header but Newtype.scala doesn't?

Nope, just copy and paste, Kai suggested we try to standardize on something in a future PR, possibly with sbt-header or something like that :)

ceedubs · 2018-02-15T03:31:51Z

core/src/main/scala/cats/data/NonEmptySet.scala

+    s.asInstanceOf[SortedSet[A]]
+
+
+  def fromSet[A: Order](as: SortedSet[A]): Option[NonEmptySet[A]] =


This takes an implicit Order[A] but then doesn't actually use it, does it? That seems misleading to me.

Yeah that's true, it assumes there's an Ordering for A, maybe best to get rid of it?

Is this the last bit that prevents this PR being merged?

It's done :)

kailuowang

on build green

kailuowang · 2018-03-07T19:01:58Z

@ceedubs any other issues?

kailuowang · 2018-03-08T15:20:12Z

@johnynek were all your feedback addressed?

ceedubs · 2018-03-09T03:56:09Z

core/src/main/scala/cats/data/NonEmptySet.scala

+    * Returns a new `SortedSet` containing all elements where the result of `pf` is defined.
+    */
+  def collect[B: Order](pf: PartialFunction[A, B]): SortedSet[B] = {
+    implicit val ordering = Order[B].toOrdering


Same comment as https://github.com/typelevel/cats/pull/2141/files#r173359573

I think there are some other places that this applies to this PR. I don't necessarily consider this a blocker, but I'd like us to at least decide what we want to do about this before we publish a release that has the name of the implicit parameter fixed (I think that the auto-generated one will be something like ev1).

LukaJCB · 2018-03-11T15:07:56Z

I removed the context bounds @ceedubs :)

ceedubs

Thanks a bunch @LukaJCB. Just one remaining small question and then this LGTM.

ceedubs · 2018-03-11T15:24:18Z

core/src/main/scala/cats/data/Newtype.scala

+  * Helper trait for `newtype`s. These allow you to create a zero-allocation wrapper around a specific type.
+  * Similar to `AnyVal` value classes, but never have any runtime overhead.
+  */
+trait Newtype { self =>


Does it make sense for this to be a public trait if Base and Tag are both private[data]?

Debatable IMO, but to me those two are implementation details, you're never actually going to want to access these, no?

Oh right I guess that in order to create your own type that uses Newtype you don't actually have to set Base and Tag, right?

I guess I'm partially hesitant to expose Newtype publicly because there are a lot of approaches to newtypes in the scala world and I'm not sure whether we want to take on the burden of keeping this one around and stable in cats. Especially since it's an internal implementation detail that isn't really used as an abstraction and is trivial enough that people could easily copy/paste it if they wanted it. I'd be inclined to make it private for now and if it works out well and people want it to be exposed publicly it's easier to open it up later than to make it private later. Admittedly I'm sometimes overly paranoid about this sort of thing...

I'd be okay with making them private :)

Add NonEmptySet

61f13d0

LukaJCB force-pushed the add-nonemptyset branch from 4f961a8 to 61f13d0 Compare January 8, 2018 17:22

johnynek reviewed Jan 8, 2018

View reviewed changes

Luka Jacobowitz added 2 commits January 8, 2018 18:43

Make NonEmptySet a semilattice and add extra test

5193dd1

Fix doctest

8104b40

Remove AnyVal and rename to toSortedSet

34aa71d

LukaJCB force-pushed the add-nonemptyset branch from 97f53df to 34aa71d Compare January 8, 2018 22:05

Add more docs

ff49e94

LukaJCB added the enhancement label Jan 15, 2018

LukaJCB added this to the 1.1 milestone Jan 15, 2018

Add concatMap

b46f5fe

LukaJCB requested review from kailuowang and ceedubs January 24, 2018 22:36

kailuowang previously approved these changes Jan 29, 2018

View reviewed changes

LukaJCB commented Jan 31, 2018

View reviewed changes

LukaJCB dismissed kailuowang’s stale review via 57b2792 February 5, 2018 08:58

Newtype encoding

bba1a5d

LukaJCB force-pushed the add-nonemptyset branch from 57b2792 to bba1a5d Compare February 5, 2018 09:13

kailuowang reviewed Feb 5, 2018

View reviewed changes

andyscott reviewed Feb 5, 2018

View reviewed changes

LukaJCB force-pushed the add-nonemptyset branch from aa29d79 to b2109ee Compare February 7, 2018 13:25

kailuowang previously approved these changes Feb 8, 2018

View reviewed changes

LukaJCB dismissed kailuowang’s stale review via e197ab3 February 9, 2018 15:51

LukaJCB force-pushed the add-nonemptyset branch 2 times, most recently from e197ab3 to 86d394a Compare February 9, 2018 16:05

Move newtype to its own file

65fe31b

LukaJCB force-pushed the add-nonemptyset branch from 86d394a to 65fe31b Compare February 11, 2018 13:21

ceedubs reviewed Feb 11, 2018

View reviewed changes

Address feedback

a5da265

ceedubs reviewed Feb 15, 2018

View reviewed changes

Remove Order constraint for fromSet constructor

0185601

kailuowang previously approved these changes Mar 5, 2018

View reviewed changes

ceedubs reviewed Mar 9, 2018

View reviewed changes

Remove Context bounds

9d16f11

LukaJCB dismissed kailuowang’s stale review via 9d16f11 March 11, 2018 15:07

ceedubs reviewed Mar 11, 2018

View reviewed changes

Make newtype private

b709773

kailuowang approved these changes Mar 14, 2018

View reviewed changes

kailuowang merged commit 3503cea into typelevel:master Mar 14, 2018

LukaJCB deleted the add-nonemptyset branch March 14, 2018 14:51

ceedubs mentioned this pull request Mar 24, 2018

NonEmptySet data structure? #123

Closed


		import scala.collection.immutable._

		final class NonEmptySet[A] private (val set: SortedSet[A]) extends AnyVal {

		checkAll("NonEmptySet[String]", BandTests[NonEmptySet[String]].band)
		checkAll("NonEmptySet[String]", EqTests[NonEmptySet[String]].eqv)


		final class NonEmptySet[A] private (val set: SortedSet[A]) {

		private implicit def ordering: Ordering[A] = set.ordering



		def of[A: Order](a: A, as: A*): NonEmptySet[A] =
		create(SortedSet(a)(Order[A].toOrdering) ++ SortedSet(as: _*)(Order[A].toOrdering) + a)

		s.asInstanceOf[SortedSet[A]]


		def fromSet[A: Order](as: SortedSet[A]): Option[NonEmptySet[A]] =

Add NonEmptySet #2143

Add NonEmptySet #2143

Conversation

LukaJCB commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 8, 2018 • edited Loading

Codecov Report

LukaJCB commented Jan 16, 2018

kailuowang left a comment

Choose a reason for hiding this comment

kailuowang commented Jan 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyscott Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

LukaJCB commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang left a comment

Choose a reason for hiding this comment

kailuowang commented Mar 7, 2018

kailuowang commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB commented Mar 11, 2018

ceedubs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 8, 2018 •

edited

Loading

kailuowang commented Jan 29, 2018 •

edited

Loading

andyscott Feb 5, 2018 •

edited

Loading