Add unidoc directive for markdowns #24563

richardimaoka · 2018-02-18T08:20:31Z

Closes #24426

Generated page under my firebase project:
https://doc.akka.io/docs/akka/current/cluster-client.html#cluster-client

I want to:

add @unidoc in akka, and make sure it works for all .md files in akka which would take time
then after 1. move the code for @unidoc to either akka-paradox or paradox

is that ok? or should we just do 2?

Copied UnidocDirective from akka/akka-http#1579, and added UnidocRefDirective to avoid the issue described here

As the first step, modified cluster-client.md to see how @unidoc works.

richardimaoka · 2018-02-18T08:22:24Z

akka-docs/src/main/paradox/cluster-client.md

 another cluster. It only needs to know the location of one (or more) nodes to use as initial
-contact points. It will establish a connection to a @scala[@scaladoc[`ClusterReceptionist`](akka.cluster.client.ClusterReceptionist)]@java[@javadoc[`ClusterReceptionist`](akka.cluster.client.ClusterReceptionist)] somewhere in
+contact points. It will establish a connection to a @unidocRef[ClusterReceptionist](akka.cluster.client.ClusterReceptionist) somewhere in


Using @unidocRef instead of @unidoc, because @unidoc gave the following error:

Error writing content for page cluster-client.html: 2 matches found for ClusterReceptionist, but not
javadsl/scaladsl: akka.cluster.client.ClusterReceptionist,
akka.cluster.typed.internal.receptionist.ClusterReceptionist

Matched two classes are not javadsl/scaladsl variants.

Although @unidoc by UnidocDirective works nicely with a concise syntax, it doesn't work in cases like above. Hence this syntax, @unidocRef[ClusterReceptionist](akka.cluster.client.ClusterReceptionist).

I agree being able to point to a particular class when unidoc can't figure it out on its own is useful. Could we even do that in one directive, so allowing @unidoc[ClusterReceptionist](akka.cluster.client.ClusterReceptionist) or perhaps even @unidoc[client.ClusterReceptionist]?

I think handling both @unidoc[ClassName] and @unidoc[ClassName](path.to.ClassName) is difficult, as we need to

extract if (path.to.ClassName) part exists,

and if 1 is not found, fall back to the current logic for @unidoc[ClassName]

The problem with 1 is that we need the same logic as SourceDirective's resolvedSource, which is sealed inside Paradox.

For @unidoc[client.ClusterReceptionist], I can give it a try. Hopefully that does not clutter the code to extract the label and filtering logic here..

@unidoc[client.ClusterReceptionist] was actually almost already handled. So I only made a slight change.

You can see the generated page here

richardimaoka · 2018-02-18T08:23:10Z

project/ParadoxSupport.scala

+    }.value
+  )
+
+  class UnidocDirective(allClasses: IndexedSeq[String]) extends InlineDirective("unidoc") {


This is copied from akka/akka-http#1579 removing unnecessary stuff.

What unnecessary stuff could be removed? can we also remove it over there?

Unnecessary stuff was removed in 4161175 - unused parameters, and case 2 in the pattern match could be handled by case n.

As we are making the new sbt plugin anyway, we don't need to do the same in akka-http? However, if the plugin takes too much time, yes I'll do that in akka-http.

richardimaoka · 2018-02-18T08:32:51Z

project/ParadoxSupport.scala

+    }
+  }
+
+  class UnidocRefDirective(page: Page, variables: Map[String, String]) extends InlineDirective("unidocRef") {


See this comment for the motivation.

akka-ci · 2018-02-18T09:18:30Z

Test PASSed.

johanandren · 2018-02-19T08:14:48Z

This is very nice!

As the javadsl/scaladsl is our Akka-specific convention I don't think we can move the directive to paradox general (unless we make it more configurable). Maybe start with the akka-paradox module so that we can use it in all Akka-umbrella projects.

patriknw

Very nice! I agree with adding it to akka-paradox.

2m · 2018-02-19T11:50:01Z

sbt-paradox-akka is only used in akka and akka-http. So it would not benefit projects like alpakka and akka-management.

We should package the directive as a separate sbt plugin. I do not think we have yet seen a paradox directive pacakged separately as an sbt plugin, but it seems to be doable.

richardimaoka · 2018-02-20T00:04:18Z

I can work on the sbt plugin later, but is it ok to firstly add the @unidoc implementation inside the akka repository?

Otherwise, I need to make sure the @unidoc implementation works for all the .md files in akka, to avoid multiple releases of the sbt plugin just to fix issues as I keep updating .md. I prefer incrementally updating .md, at least in a couple of PRs, but if we should get the sbt plugin ready first, and go with a single big PR to convert all .md, i follow that approach.

raboof · 2018-02-20T09:18:42Z

Incubating it in the project and extracting it later seems fine to me

richardimaoka · 2018-02-23T12:27:54Z

project/ParadoxSupport.scala

          })
        case n =>
          throw new java.lang.IllegalStateException(s"$n matches found for $label, but not javadsl/scaladsl: ${matches.mkString(", ")}")
      }
    }
  }
-
-  class UnidocRefDirective(page: Page, variables: Map[String, String]) extends InlineDirective("unidocRef") {


It turned out @unidocRef is not needed as @unidoc[path.to.ClassName] was already handled.

richardimaoka · 2018-02-23T12:30:03Z

Think it's good for another round of review.

akka-ci · 2018-02-23T12:39:19Z

Test PASSed.

johanandren · 2018-02-23T13:01:55Z

akka-docs/src/main/paradox/cluster-client.md

@@ -3,7 +3,7 @@
 An actor system that is not part of the cluster can communicate with actors
 somewhere in the cluster via this @unidoc[ClusterClient]. The client can of course be part of
 another cluster. It only needs to know the location of one (or more) nodes to use as initial
-contact points. It will establish a connection to a @unidocRef[ClusterReceptionist](akka.cluster.client.ClusterReceptionist) somewhere in
+contact points. It will establish a connection to a @unidoc[client.ClusterReceptionist] somewhere in


This is a bit strange though, it's neither the FQCN nor the simple name but something in-between, I think either @unidoc[ClusterReceptionist] or @unidoc[akka.cluster.client.ClusterReceptionist] is what we'd want

OK, added a new commit to only allow FQCN or a simple class name (without any .).

richardimaoka · 2018-02-24T02:02:29Z

project/ParadoxSupport.scala

+        printer.print(s"</span>")
+      } catch {
+        case e: LinkException =>
+          throw new LinkException(s"${e.getMessage}. You may want to use the fully qualified class name as `@unidoc[fqcn]`.")


Error message is like:

[error] Caused by: com.lightbend.paradox.markdown.ExternalLinkDirective$LinkException: Failed to resolve [client.ClusterReceptionist] referenced from [cluster-client.html] because property [javadoc.base_url] is not defined. You may want to use the fully qualified class name as @unidoc[fqcn].
[error] at akka.ParadoxSupport$UnidocDirective.renderByFqcn(ParadoxSupport.scala:94)

akka-ci · 2018-02-24T02:40:59Z

Test PASSed.

richardimaoka · 2018-02-24T03:26:07Z

project/ParadoxSupport.scala

+        case n =>
+          throw new java.lang.IllegalStateException(
+            s"$n matches found for @unidoc[$label], but not javadsl/scaladsl: ${matches.mkString(", ")}. " +
+              s"You may want to use the fully qualified class name as @unidoc[fqcn] instead of @unidoc[${label}]."


Error message is like:

[error] Caused by: java.lang.IllegalStateException: 2 matches found for @unidoc[ClusterReceptionist], but not javadsl/scaladsl: akka.cluster.client.ClusterReceptionist, akka.cluster.typed.internal.receptionist.ClusterReceptionist. You may want to use the fully qualified class name as @unidoc[fqcn] instead of @unidoc[ClusterReceptionist].
[error] at akka.ParadoxSupport$UnidocDirective.renderByClassName(ParadoxSupport.scala:78)

akka-ci · 2018-02-24T04:29:41Z

Test PASSed.

akka-ci · 2018-02-26T16:11:14Z

Test FAILed.

jrudolph · 2018-02-26T16:11:55Z

(Ignore the latest failure, I was testing a Jenkins thing)

akka-ci · 2018-02-26T16:16:29Z

Test FAILed.

jrudolph · 2018-02-26T16:17:20Z

PLS BUILD

akka-ci · 2018-02-26T17:19:41Z

Test PASSed.

raboof · 2018-02-28T09:36:39Z

project/ParadoxSupport.scala

+      }
+    }
+
+    def renderByClassName(label: String, node: DirectiveNode, visitor: Visitor, printer: Printer): Unit = {


It would be neat if this logic would just find the fqcn based on the short text and then re-use renderByFqcn, would that be possible?

I iterated further on this a bit in akka/akka-http#1890, could you have a look whether what I did there make sense and if so apply the same here?

Sure, copied UnidocDirective from akka/akka-http#1890

akka-ci · 2018-02-28T21:57:28Z

Test PASSed.

raboof

Super nice!

johanandren

LGTM, thanks a bunch!

richardimaoka added 4 commits February 18, 2018 17:05

Copy UnidocDirective from akka-http

b7e7509

Remove unnecessary stuff and fix issues

4161175

Add @unidocRef[ClassName](package.to.ClassName)

662e2bc

cluster-client.md using @unidoc and @unidocRef

ad9843d

akka-ci added the validating PR is currently being validated by Jenkins label Feb 18, 2018

richardimaoka commented Feb 18, 2018

View reviewed changes

richardimaoka changed the title ~~Unidoc richard~~ Add unidoc directive for markdowns Feb 18, 2018

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Feb 18, 2018

patriknw reviewed Feb 19, 2018

View reviewed changes

Both @unidoc[ClassName] and @unidoc[path.to.ClassName] works

28824b6

akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Feb 23, 2018

richardimaoka commented Feb 23, 2018

View reviewed changes

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Feb 23, 2018

johanandren reviewed Feb 23, 2018

View reviewed changes

Allow only FQCN or simple class name (i.e. without packages)

7d41a33

akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Feb 24, 2018

richardimaoka commented Feb 24, 2018

View reviewed changes

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Feb 24, 2018

Error message tweaking

d8d1214

richardimaoka commented Feb 24, 2018

View reviewed changes

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Feb 24, 2018

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Feb 26, 2018

johanandren mentioned this pull request Feb 27, 2018

Upgrade to Akka 2.5.10+ - unidoc troubles akka/akka-http#1889

Closed

raboof reviewed Feb 28, 2018

View reviewed changes

Copy from akka/akka-http#1890

ad4ec77

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Feb 28, 2018

raboof approved these changes Mar 1, 2018

View reviewed changes

johanandren approved these changes Mar 1, 2018

View reviewed changes

johanandren merged commit 82f50a8 into akka:master Mar 1, 2018

Add unidoc directive for markdowns #24563

Add unidoc directive for markdowns #24563

Conversation

richardimaoka commented Feb 18, 2018 • edited Loading

richardimaoka Feb 18, 2018 • edited Loading

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

akka-ci commented Feb 18, 2018

johanandren commented Feb 19, 2018

patriknw left a comment

Choose a reason for hiding this comment

2m commented Feb 19, 2018

richardimaoka commented Feb 20, 2018

raboof commented Feb 20, 2018

Choose a reason for hiding this comment

richardimaoka commented Feb 23, 2018

akka-ci commented Feb 23, 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

akka-ci commented Feb 24, 2018

Choose a reason for hiding this comment

akka-ci commented Feb 24, 2018

akka-ci commented Feb 26, 2018

jrudolph commented Feb 26, 2018

akka-ci commented Feb 26, 2018

jrudolph commented Feb 26, 2018

akka-ci commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Feb 28, 2018

raboof left a comment

Choose a reason for hiding this comment

johanandren left a comment

Choose a reason for hiding this comment

richardimaoka commented Feb 18, 2018 •

edited

Loading

richardimaoka Feb 18, 2018 •

edited

Loading