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

Fix build for scaladoc generation of Laws #961

Closed
wants to merge 1 commit into from

Conversation

mikejcurry
Copy link
Contributor

This PR changes a number of laws classes to not use syntax, and updates build.sbt to enable scaladoc generation for the law project.

The problem outlined in #516 only occur if syntax is used to reference the operations on Apply and Foldable. This PR changes these references to use the type classes directly instead of using the syntax.

Once this is in place, the scaladoc generates correctly for the laws project during the makeSite project.

This PR would fix #510 as it publishes the scaladoc for laws as part of the site.

The fix is a bit ugly, because specific syntax has to be avoided, and a deeper investigation into how to make that syntax work with scaladoc is probably required, but given where things are, this is probably a useful step forward, despite the kludginess.

Note that this has conflicts with both #959 and #960 so will need to merge to latest once those have been merged.

@@ -2,7 +2,7 @@ package cats
package laws

/**
* Laws that must be obeyed by any `Bimonad`.
* Laws that must be obeyed by any [[Bimonad]].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added this to show that this compiles correctly during site generation and generates a link to Bimonad from BimonadLaws during site generation. This example was specifically referenced in #516

@codecov-io
Copy link

Current coverage is 90.93%

Merging #961 into master will decrease coverage by -0.04% as of 94f6cb8

@@            master    #961   diff @@
======================================
  Files          180     180       
  Stmts         2150    2150       
  Branches        42      42       
  Methods          0       0       
======================================
- Hit           1956    1955     -1
  Partial          0       0       
- Missed         194     195     +1

Review entire Coverage Diff as of 94f6cb8

Powered by Codecov. Updated on successful CI builds.

@ghost
Copy link

ghost commented Mar 29, 2016

Out of interest, what exactly is the cause of the scaladoc breaking the builds? By that, I mean is it scaladoc/unidoc/doctest/makesite...?

I ask just to be sure that the correct thing is being disabled in the build. For example, if it is doctest that is failing, we could just disable that in 2.10 and still generate the docs

@mikejcurry
Copy link
Contributor Author

Hey, thanks for getting back. Yea, for sure, I guess I raised this one as much for discussion as anything else. If there are better options we should definitely go with those.

The other 2 PRs I sent were to disable scaladoc for 2.10 only. If theres a better way to do that, then all for it - they stop a problem which happens for macro expansion in 2.10 when generating scaladoc for Comonad - i.e. #898, to try to help that one add some docs for Comonad. Other projects have mentioned they have removed 2.10 from unidoc generation also, hence those PRs

But that's a different issue to this one.

This one references an issue that's been around for a while. The issue #510 is basically stating that the laws scaladoc should be published, because it is an important part of cats. The problem there affects scala 2.11. Being honest, I'm not sure what the underlying cause is. It may not be related to macro expansion at all. It only seems to affect two syntax types, ApplySyntax and FoldableSyntax. Other syntax types used in the laws code seem fine when generating docs. It could be related to the Unapply classes that are used when constructing the syntax for Foldable and Apply.

So basically, when I noticed that it only relates to the syntax for those two type classes, I put this PR together to show that it could be resolved by avoiding the syntax for those two type classes.

So, this definitely solves #510. But it is kludgy and if there is a different way to solve it without avoiding the syntax issues, I definitely think we should do that.

@ghost
Copy link

ghost commented Mar 29, 2016

@mikejcurry Might be worth a look to see if the setting I mention helps somehow, even if other issues still exist

@mikejcurry
Copy link
Contributor Author

Yea, I'd seen that alright, but couldn't find a way to get it to work with cats.

Unfortunately, -Ymacro-no-expand (or as that is deprecated, the current approach of -Ymacro-expand:none), results in a pile of errors like the below. I don't think that disabling macro expansion will be an option given how much macros are used in the core of the definition of the library.

[error] /Users/user/repos/open_source/cats/core/src/main/scala/cats/syntax/coflatMap.scala:11: not found: value CoflatMap
[error] trait CoflatMapSyntax extends CoflatMap.ToCoflatMapOps with CoflatMapSyntax1
[error]                               ^
[error] /Users/user/repos/open_source/cats/core/src/main/scala/cats/syntax/comonad.scala:12: not found: value Comonad
[error] trait ComonadSyntax extends Comonad.ToComonadOps with ComonadSyntax1
[error]                             ^
[error] /Users/user/repos/open_source/cats/core/src/main/scala/cats/syntax/contravariant.scala:15: type ToContravariantOps is not a member of object cats.functor.Contravariant
[error] trait ContravariantSyntax extends Contravariant.ToContravariantOps with ContravariantSyntax1
[error]                                                 ^
[error] /Users/user/repos/open_source/cats/core/src/main/scala/cats/syntax/foldable.scala:12: type ToFoldableOps is not a member of object cats.Foldable
[error] trait FoldableSyntax extends Foldable.ToFoldableOps with FoldableSyntax1 {
[error]                                       ^

@mikejcurry
Copy link
Contributor Author

Hey,
So, I merged this up with the latest to resolve conflicts.

I realize the changes might be a bit contentious. As far as I can ascertain, there is something that goes wrong with the application of Unapply when the compiler runs in scaladoc mode, which means that the syntax for the Foldable and Apply type classes that requires Unapply in order to be found, are not resolved in that compiler mode which in turn means that any references to this syntax fails to compile in scaladoc/unidoc generation mode.

The end result of all that is that the scaladoc for the laws project can't be published.

The way I see it, this workaround at least allows the scaladoc for the laws project to be published, which is a good step forward. The limitations related to the syntax and the scaladoc references could be followed up separately.

@mikejcurry
Copy link
Contributor Author

Kind of hoping that the amazing higher order unification work that has been going on makes this proposed workaround obsolete. I can close this if folks prefer, but might be worth keeping around for a while as a PR until an alternative is found. If the need for Unapply goes away, then I'd hope that the scaladoc generation which is failing in this area might just start working. :-).

@ghost
Copy link

ghost commented May 22, 2016

I cannot be certain, but akka/akka#20533 may be a similar issue,
as the errors are similar :

[error] value ap is not a member of type parameter F[B => C]
[error] value ap is not a member of type parameter F[A => B]
[error] Unused import
[error] import cats.syntax.apply._

ie the errors can be reproduced in the library compile build by removing the syntax import

@eed3si9n Any thoughts on this, please?

@eed3si9n
Copy link
Contributor

eed3si9n commented May 23, 2016

Unifying the source seem to interfere with implicits and macros.
What I've done in the past is workaround this by creating a branch and sprinkling something like:

implicit def fasttypetag[A]: FastTypeTag[A] = ???

liberally in the source, since you just need to get the code to compile to generate the docs.
Maybe you can create object DocHacks, which remains empty for normal compilation, but during the doc does all the hacks, and call import DocHacks._.

@ghost
Copy link

ghost commented May 23, 2016

@eed3si9n Firstly, thanks for the feedback, very much appreciated.

I wonder, when you say "Unifying the source seem...." if this would work better if we did not use unidoc for now (we have no Java code)? Or would it not make any difference?

@ghost ghost mentioned this pull request Jun 1, 2016
@mikejcurry
Copy link
Contributor Author

Closing in anticipation of the great work in #1082 !!! Great stuff @inthenow

@mikejcurry mikejcurry closed this Jun 1, 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.

Laws are missing in the published API docs
4 participants