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

Topic/scaladoc coflat comonad #131 #898

Merged
merged 4 commits into from
May 30, 2016

Conversation

lukewyman
Copy link
Contributor

Added scaladoc comments with REPL examples.

@mikejcurry
Copy link
Contributor

Seems to be a variation of typelevel/simulacrum#9 and #6. Though those particular issues seems to be resolved, macro expansion for Comonad, during scaladoc generation, on scala 2.10.6, seems to break when any documentation is added to extract.

CoflatMap seems fine, and does not exhibit the problem.
Scala 2.11 seems fine, and does not exhibit the problem.

The following cut back example for Comonad causes the same issue when running docs/makeSite for scala 2.10.6:

package cats

import simulacrum.typeclass

/**
 * Must obey the laws defined in cats.laws.ComonadLaws.
 */
@typeclass trait Comonad[F[_]] extends CoflatMap[F] {
  /** Breaks macro expansion of the Comonad Object */
  def extract[A](x: F[A]): A
}

@lukewyman
Copy link
Contributor Author

Thanks, @mikejcurry, will take a stab at solving the docs/makeSite for 2.10.6 locally and see if I can get a build to pass after that.

@lukewyman
Copy link
Contributor Author

Hey @ceedubs, I'm sorry to say that it looks like the build fail issue on Scala 2.10.6 appears to be over my head :(

@ceedubs
Copy link
Contributor

ceedubs commented Mar 24, 2016

@mpilquist or @travisbrown do you have any ideas on this?

@travisbrown
Copy link
Contributor

@ceedubs This is kind of frightening but I have no recollection of ever having written any of the stuff I apparently wrote in #6.

Since then I've run into similar problems in the circe modules that use macros heavily, and my solution has been just to exclude them from the 2.10 doc tasks—I just haven't had time to care.

@codecov-io
Copy link

Current coverage is 90.78%

Merging #898 into master will decrease coverage by -0.04% as of 16f5ddf

@@            master    #898   diff @@
======================================
  Files          184     184       
  Stmts         2181    2182     +1
  Branches        43      43       
  Methods          0       0       
======================================
  Hit           1981    1981       
  Partial          0       0       
- Missed         200     201     +1

Review entire Coverage Diff as of 16f5ddf

Powered by Codecov. Updated on successful CI builds.

* scala> import cats.Comonad
* scala> val id = Id.pure(3)
* scala> Comonad[Id].extract(id)
* res0: cats.Id[Int] = 3
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 minor, but could we make this res0: Int = 3? I think that would make it a bit more clear that extract pulls an A out of an F context. Id is the weird case where Id[A] and A happen to be the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we'll need to change Id.pure after #955. We could either do val id: Id[Int] = 3 or we could do something like Applicative[Id].pure(3).

@mikejcurry
Copy link
Contributor

Hmm.. looks like I missed somewhere with disabling the doc generation for 2.10. :-(. It got past building the site this time, but looks like the doc generation is maybe run again as part of packaging or something. I'll have a look and see if anything jumps out.

@mikejcurry
Copy link
Contributor

I think if someone can restart the build on this it should succeed after #959 - as the last failure was a failure when building scaladoc during 2.10 publish an #959 disables that.

@ceedubs
Copy link
Contributor

ceedubs commented Apr 2, 2016

@lukewyman sorry, but it looks like there is a compile error now due to Id changes that were merged in.

@lukewyman
Copy link
Contributor Author

Thanks, @ceedubs, I'll get it cleaned up this weekend.

@ceedubs
Copy link
Contributor

ceedubs commented Apr 19, 2016

👍

@kailuowang
Copy link
Contributor

kailuowang commented Apr 27, 2016

As a fun fact, adding a dummie private val will make the doc build on 2.10.6 working.

@typeclass trait Comonad[F[_]] extends CoflatMap[F] {

  /**
    * `extract` is the dual of `pure` on Monad (via `Applicative`)
    *  ....
    */
  def extract[A](x: F[A]): A

  private val dummy = ()

}

obviously, I am not suggesting this as a fix, just a fun note.

@kailuowang
Copy link
Contributor

Please note that once #1006 is merged, this one would be breaking the build again, please rerun the build to reflect that.
A couple of options to consider to fix this one:

  1. find a more comprehensive way to disable doc generation for 2.10.6 - this requires change to sonatype release process to bypass javadoc jar uploads. ( I spent about 2 hours , couldn't find out how to tweak sbt-sonatype that way)
  2. temporarily disable the scaladoc to extract by changing /** ... */ to /* .. */ and wait for macro paradise or scala fix.
  3. find other tricks that could magically fix this compilation error, tricks such as in Fix build for scaladoc generation of Laws #961

@kailuowang
Copy link
Contributor

update to my previous comment, as this comment pointed out:
http://central.sonatype.org/pages/requirements.html#supply-javadoc-and-sources suggested that

If, for some reason (for example, license issue or it's a Scala project), you can not provide -sources.jar or -javadoc.jar , please make fake -sources.jar or -javadoc.jar with simple README inside to pass the checking.

So #959 (already reverted) would have to create some fake javadoc.jar in the release process.

@non
Copy link
Contributor

non commented May 30, 2016

This seems great to me. 👍

@non non merged commit 2ab7b93 into typelevel:master May 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants