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 doctests for Semigroup, Group, and Monoid #2523

Merged
merged 4 commits into from
Sep 26, 2018

Conversation

DennisVDB
Copy link
Contributor

PR for part of #2479.

Adds doctests for the Semigroup, Group, and Monoid typeclasses.

@DennisVDB
Copy link
Contributor Author

sbt validate is failing with:

error] /Users/dennis/Documents/Programming/scala/cats/kernel/.jvm/target/scala-2.12/src_managed/test/cats/kernel/GroupDoctest.scala:4:19: object scalacheck is not a member of package org
[error] import _root_.org.scalacheck.Prop._
[error]                   ^
[error] /Users/dennis/Documents/Programming/scala/cats/kernel/.jvm/target/scala-2.12/src_managed/test/cats/kernel/GroupDoctest.scala:7:24: object scalacheck is not a member of package org
[error]     extends _root_.org.scalacheck.Properties("Group.scala") {
[error]                        ^
[error] /Users/dennis/Documents/Programming/scala/cats/kernel/.jvm/target/scala-2.12/src_managed/test/cats/kernel/GroupDoctest.scala:7:46: no arguments allowed for nullary constructor Object: ()Object
[error]     extends _root_.org.scalacheck.Properties("Group.scala") {
[error]                                              ^
[error] /Users/dennis/Documents/Programming/scala/cats/kernel/.jvm/target/scala-2.12/src_managed/test/cats/kernel/GroupDoctest.scala:15:3: not found: value include
[error]   include(new _root_.org.scalacheck.Properties("Group.scala:10: inverse") {
...

@kailuowang
Copy link
Contributor

kailuowang commented Sep 22, 2018

probably need to add this setting to the kernel settings

.settings(libraryDependencies += "org.scalacheck" %%% "scalacheck" % scalaCheckVersion(scalaVersion.value) % "test")

* {{{
* scala> import cats.implicits._
*
* scala> Monoid[String].combineAllOption(List("One ", "Two ", "Three"))
Copy link
Member

Choose a reason for hiding this comment

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

Don't you mean combineAll here?

@DennisVDB
Copy link
Contributor Author

@kailuowang it doesn't seem to fix the problem.

Locally I'm using the updated build.sbt from this PR #2382 as I couldn't import the project in IntelliJ. But that shouldn't influence the fact that travis fails.

@kailuowang
Copy link
Contributor

I might missed something but I don't see build.sbt updated

@DennisVDB
Copy link
Contributor Author

You didn't miss it.

I now pushed the updated build.sbt with the fix you suggested but it still fails the same way as before. It seems that the code generated for the doctest doesn't compile.

The generated code: /cats/kernel/.jvm/target/scala-2.12/src_managed/test/cats/kernel/*Doctest.scala

Travis failure: https://travis-ci.org/typelevel/cats/jobs/432625136
Local failure: https://gist.github.com/DennisVDB/1fa934962f2f192af80731b1336adb1d

Copy link
Contributor

@larsrh larsrh left a comment

Choose a reason for hiding this comment

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

You have to change the imports to be more specific and use the objects where the instances are defined, like so:

$ git diff
diff --git a/kernel/src/main/scala/cats/kernel/Group.scala b/kernel/src/main/scala/cats/kernel/Group.scala
index c792245e..31d01914 100644
--- a/kernel/src/main/scala/cats/kernel/Group.scala
+++ b/kernel/src/main/scala/cats/kernel/Group.scala
@@ -14,7 +14,7 @@ trait Group[@sp(Int, Long, Float, Double) A] extends Any with Monoid[A] {
    *
    * Example:
    * {{{
-   * scala> import cats.implicits._
+   * scala> import cats.kernel.instances.int._
    *
    * scala> Group[Int].inverse(5)
    * res0: Int = -5
@@ -29,7 +29,7 @@ trait Group[@sp(Int, Long, Float, Double) A] extends Any with Monoid[A] {
    *
    * Example:
    * {{{
-   * scala> import cats.implicits._
+   * scala> import cats.kernel.instances.int._
    *
    * scala> Group[Int].remove(5, 2)
    * res0: Int = 3

This is a bit annoying, but I don't see any way around it, short of defining a cats.kernel.implicits._ import object.

*
* Example:
* {{{
* scala> import cats.implicits._
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that cats.implicits is only defined in cats-core, not in cats-kernel, so importing it here won't work.

Copy link
Contributor

@larsrh larsrh left a comment

Choose a reason for hiding this comment

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

Pending successful build, approval from me.

@codecov-io
Copy link

codecov-io commented Sep 25, 2018

Codecov Report

Merging #2523 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2523      +/-   ##
==========================================
- Coverage   95.35%   95.34%   -0.02%     
==========================================
  Files         358      358              
  Lines        6530     6535       +5     
  Branches      282      279       -3     
==========================================
+ Hits         6227     6231       +4     
- Misses        303      304       +1
Impacted Files Coverage Δ
kernel/src/main/scala/cats/kernel/Semigroup.scala 68.75% <ø> (ø) ⬆️
kernel/src/main/scala/cats/kernel/Group.scala 71.42% <ø> (ø) ⬆️
kernel/src/main/scala/cats/kernel/Monoid.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/eq.scala 0% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Validated.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Ior.scala 98.5% <0%> (+0.02%) ⬆️
core/src/main/scala/cats/data/NonEmptyChain.scala 98.98% <0%> (+1.01%) ⬆️

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 1d5a51a...eb44c8a. Read the comment docs.

@kailuowang kailuowang merged commit 8c891ee into typelevel:master Sep 26, 2018
@kailuowang kailuowang added this to the 1.5 milestone Oct 30, 2018
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.

5 participants