Restructure and document First/Last and Min/Max #578

sritchie · 2016-11-24T18:50:07Z

This PR:

Creates Gen.scala with generators for Algebird data structures
Creates Arbitrary.scala with arbitrary instances for Algebird data structures
Moves the generators for Interval, First, Last, Min, Max and ExpHist into these objects
moves the data structures from OrderedSemigroup.scala into their own files
adds full tests for min, max, first, last
adds + operator to min, max, first, last
adds min operator to Min and max operator to Max
adds FirstAggregator and LastAggregator
adds Monoid[Max[Vector[T]]] and Monoid[Max[Stream[T]]]
adds tut-based documentation for first, last, min and max.

This sets up the structure I need for upcoming documentation PRs. With each PR I'd like to spruce up each data structure, move its tests into its own file, get the documentation looking good and add arbitrary and gen instances to Gen and Arbitrary.

Following PRs should be smaller now that the structure for this is in place.

sritchie

Comments on what I've got so far.

sritchie · 2016-11-24T18:50:29Z

algebird-core/src/main/scala/com/twitter/algebird/Max.scala

  implicit def semigroup[T](implicit ord: Ordering[T]): Semigroup[Max[T]] =
    Semigroup.from[Max[T]] { (l, r) => if (ord.gteq(l.get, r.get)) l else r }

+  implicit def ordering[T](implicit ord: Ordering[T]): Ordering[Max[T]] =


ah, these gets ordering instances as well.

sritchie · 2016-11-24T18:50:59Z

algebird-test/src/main/scala/com/twitter/algebird/scalacheck/ExpHistInstances.scala

+import org.scalacheck.Arbitrary.{ arbitrary => getArbitrary }
+import Gen.oneOf
+
+trait ExpHistGen {


what do you think of this style, vs jamming everything into a single file?

sritchie · 2016-11-24T18:52:08Z

algebird-test/src/test/scala/com/twitter/algebird/CollectionSpecification.scala

 class CollectionSpecification extends CheckProperties {
  import com.twitter.algebird.BaseProperties._
-
-  implicit def arbMin[T: Arbitrary]: Arbitrary[Min[T]] =


moved to Arbitrary.scala`

sritchie · 2016-11-24T18:52:17Z

algebird-test/src/test/scala/com/twitter/algebird/CollectionSpecification.scala

-  implicit def arbAndVal: Arbitrary[AndVal] =
-    Arbitrary { implicitly[Arbitrary[Boolean]].arbitrary.map{ b => AndVal(b) } }
-
-  property("MinSemigroup is a commutative semigroup") {


moved into Min and Max tests

sritchie · 2016-11-24T18:52:33Z

algebird-test/src/test/scala/com/twitter/algebird/ExpHistLaws.scala

  }
 }

-object ExpHistGenerators {


moved to ExpHistInstances.scala in the scalacheck package

sritchie · 2016-11-24T18:52:49Z

algebird-test/src/test/scala/com/twitter/algebird/Generators.scala

-
-case class PosNum[T: Numeric](value: T)
-
-object PosNum {


All this is moved to Gen.scala and Arbitrary.scala

sritchie · 2016-11-24T18:53:16Z

algebird-test/src/test/scala/com/twitter/algebird/Generators.scala

-object Generators {
-  // The new scalacheck oneOf results in diverging implicits here
-  // should follow up an investigation?
-  def oneOf[T](g1: Gen[T], g2: Gen[T], gs: Gen[T]*) = for {


I managed to get rid of this diverging implicit expansion by adding a [T] parameter onto the intervalArb sub-generators.

sritchie · 2016-11-24T18:53:48Z

algebird-test/src/test/scala/com/twitter/algebird/MaxSpec.scala

+  }
+
+  property("Max[Long] is a commutative monoid") {
+    commutativeMonoidLaws[Max[Long]]


bumped these up to commutative monoid tests from the old monoid-only tests.

sritchie · 2016-11-24T18:54:17Z

algebird-test/src/test/scala/com/twitter/algebird/MinSpec.scala

+import com.twitter.algebird.scalacheck.NonEmptyVector
+import org.scalacheck.Prop.forAll
+
+class MinSpec extends CheckProperties {


tests work great as long as you extend CheckProperties.

sritchie · 2016-11-24T18:54:29Z

docs/src/main/tut/datatypes/first_and_last.md


+`First` and `Last`.

+## Algebraic Properties


codecov-io · 2016-11-24T19:08:07Z

Current coverage is 65.21% (diff: 92.30%)

Merging #578 into develop will increase coverage by 0.77%

@@            develop       #578   diff @@
==========================================
  Files           111        119     +8   
  Lines          4572       4651    +79   
  Methods        4154       4221    +67   
  Messages          0          0          
  Branches        379        391    +12   
==========================================
+ Hits           2946       3033    +87   
+ Misses         1626       1618     -8   
  Partials          0          0

Powered by Codecov. Last update 776d7a6...e1cbebb

morazow · 2016-11-26T22:26:12Z

algebird-test/src/test/scala/com/twitter/algebird/LastSpec.scala

+    }
+  }
+
+  property("Last.aggregator returns the first item") {


Hello @sritchie,
I was just checking this pr. I think the above statement meant to be ".. last item".

Ah, thank you @morazow! I'll make the change tomorrow when I add the actual documentation updates.

sritchie · 2016-11-27T20:41:45Z

algebird-core/src/main/scala/com/twitter/algebird/First.scala

+  implicit def semigroup[T]: Semigroup[First[T]] = new Semigroup[First[T]] {
+    def plus(l: First[T], r: First[T]): First[T] = l
+
+    override def sumOption(iter: TraversableOnce[First[T]]): Option[First[T]] =


should be more efficient.

sritchie · 2016-11-27T20:42:13Z

algebird-core/src/main/scala/com/twitter/algebird/Max.scala

+    0
+  }
+
+  implicit def vectorMonoid[T: Ordering]: Monoid[Max[Vector[T]]] =


I followed cats here by only implementing this for Stream and Vector in addition to list.

sritchie · 2016-11-28T00:37:01Z

@johnynek if we can get #579 in we can close this, since that PR builds on this one and you reviewed everything that's in here already.

sritchie · 2016-11-28T00:44:38Z

Closed in favor of #529

restructure code for first, last, min, max

7d9c73f

sritchie added documentation testing labels Nov 24, 2016

abstract exphist instances

eb33436

sritchie force-pushed the sritchie/ordering_docs branch from b2f31b0 to eb33436 Compare November 24, 2016 18:52

sritchie commented Nov 24, 2016

View reviewed changes

sritchie-stripe added 3 commits November 24, 2016 15:10

sumOption for first

837a054

add instances

88eb83f

add equiv

77ef164

morazow reviewed Nov 26, 2016

View reviewed changes

sritchie-stripe added 2 commits November 27, 2016 11:08

add docs

7a57443

add vector and stream

de12da4

sritchie changed the title ~~[WIP] Restructure and document First/Last and Min/Max~~ Restructure and document First/Last and Min/Max Nov 27, 2016

sritchie commented Nov 27, 2016

View reviewed changes

more missing

e1cbebb

sritchie mentioned this pull request Nov 27, 2016

Lots of documentation + test fixes, updates #579

Merged

sritchie closed this Nov 28, 2016

Restructure and document First/Last and Min/Max #578

Restructure and document First/Last and Min/Max #578

Uh oh!

Conversation

sritchie commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sritchie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 65.21% (diff: 92.30%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sritchie commented Nov 28, 2016

Uh oh!

sritchie commented Nov 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sritchie commented Nov 24, 2016 •

edited

Loading

codecov-io commented Nov 24, 2016 •

edited

Loading