apache / pekko

Build highly concurrent, distributed, and resilient message-driven applications using Java/Scala
https://pekko.apache.org/
Apache License 2.0
1.17k stars 140 forks source link

lots of scaladoc warnings in build #353

Open pjfanning opened 1 year ago

pjfanning commented 1 year ago

Check out git repo and run sbt unidoc and you will lots of warnings like Could not find any member to link for (and others).

It would be great if anyone who want to help can look at these and submit some PRs.

ydash commented 1 year ago

@pjfanning May I work on it?

pjfanning commented 1 year ago

@ydash - this is still an issue. If you have time to contribute some PRs that would be great.

We are looking to do a release at the moment so we would like to minimise disruption. Could you keep any PRs small? Not one change in each PR but a relatively limited number of changes in each PR.

ydash commented 1 year ago

It seems to appear two types of warn log, Could not find any member to link and The link target is ambiguous.

One of Could not find any member to link logs is following:

`[warn] \path\incubator-pekko\cluster-sharding-typed\src\main\scala\org\apache\pekko\cluster\sharding\typed\testkit\javadsl\EntityRef.scala:22:1: Could not find any member to link for "EntityRef".`

This problem can be resolved by explicitly writing a full path of an entity:

-  * For testing purposes this `EntityRef` can be used in place of a real [[EntityRef]]. 
+  * For testing purposes this `EntityRef` can be used in place of a real [[org.apache.pekko.cluster.sharding.typed.javadsl.EntityRef EntityRef]].

https://github.com/apache/incubator-pekko/blob/966204814ecac09b9d7cc6e866fe2a71a660ab05/cluster-sharding-typed/src/main/scala/org/apache/pekko/cluster/sharding/typed/testkit/javadsl/EntityRef.scala#L23

Following is an example of The link target is ambiguous logs:

[warn] \path\incubator-pekko\actor-testkit-typed\src\main\scala\org\apache\pekko\actor\testkit\typed\javadsl\TestProbe.scala:70:1: The link target "ActorTestKit#createTestProbe" is ambiguous. Several members fit the target:
[warn] [M](name: String, clazz: Class[M]): org.apache.pekko.actor.testkit.typed.javadsl.TestProbe[M] in class ActorTestKit [chosen]
[warn] [M](name: String): org.apache.pekko.actor.testkit.typed.javadsl.TestProbe[M] in class ActorTestKit
[warn] [M](clazz: Class[M]): org.apache.pekko.actor.testkit.typed.javadsl.TestProbe[M] in class ActorTestKit
[warn] [M](): org.apache.pekko.actor.testkit.typed.javadsl.TestProbe[M] in class ActorTestKit

If a link target is ambiguous, scaladoc seems to automatically generate a link for one candidate. So, I will resolve this problem by explicitly writing link of a candidate automatically chosen:

-  * or via [[ActorTestKit#createTestProbe]].
+  * or via [[ActorTestKit#createTestProbe[M](name:String,clazz:Class[M])* ActorTestKit#createTestProbe]].

https://github.com/apache/incubator-pekko/blob/966204814ecac09b9d7cc6e866fe2a71a660ab05/actor-testkit-typed/src/main/scala/org/apache/pekko/actor/testkit/typed/javadsl/TestProbe.scala#L72

@pjfanning What do you think about the above plan?

pjfanning commented 1 year ago

@ydash seems ok to me - but my point in an earlier comment still stands - can we keep any PRs a manageable size - to make them easier to review?

ydash commented 1 year ago

@pjfanning

can we keep any PRs a manageable size - to make them easier to review?

Probably yes, we can. I will divide into two PRs based on the warning log type. One will have about 10 lines modification and the other will have abount 80 lines modification.

ydash commented 1 year ago

I've found links which refer to a private type or member. Those links cause Could not find any member to link warn logs. For example: https://github.com/apache/incubator-pekko/blob/4ce1095b4b48f5cdd31342ab9f0950bd44a2829a/actor/src/main/scala/org/apache/pekko/dispatch/Dispatchers.scala#L326

Unless specified by -private option, scaladoc generate only public and protected types and members. https://github.com/scala/scala/blob/93d6281876ae53d7eb0d1f1e8369437f0a260015/src/scaladoc/scala/tools/nsc/doc/Settings.scala#L241-L244

@pjfanning Which solution do you think is better:

  1. Add -private option to generate scaladoc of private types and members.
  2. Disable links by removing square brackets (e.g. [[pekko.dispatch.BalancingDispatcher]] to pekko.dispatch.BalancingDispatcher).
  3. Ignore warn log. Those links work well in Intellij IDEA. Developers can jump to private members and types under coding.
  4. Something else.
pjfanning commented 1 year ago

I've found links which refer to a private type or member. Those links cause Could not find any member to link warn logs. For example:

https://github.com/apache/incubator-pekko/blob/4ce1095b4b48f5cdd31342ab9f0950bd44a2829a/actor/src/main/scala/org/apache/pekko/dispatch/Dispatchers.scala#L326

Unless specified by -private option, scaladoc generate only public and protected types and members. https://github.com/scala/scala/blob/93d6281876ae53d7eb0d1f1e8369437f0a260015/src/scaladoc/scala/tools/nsc/doc/Settings.scala#L241-L244

@pjfanning Which solution do you think is better:

  1. Add -private option to generate scaladoc of private types and members.
  2. Disable links by removing square brackets (e.g. [[pekko.dispatch.BalancingDispatcher]] to pekko.dispatch.BalancingDispatcher).
  3. Ignore warn log. Those links work well in Intellij IDEA. Developers can jump to private members and types under coding.
  4. Something else.

3. Ignore warn log seems best for now

ydash commented 1 year ago

Links for java external library also cause Could not find any member to link warn logs. For example.: https://github.com/apache/incubator-pekko/blob/4ce1095b4b48f5cdd31342ab9f0950bd44a2829a/slf4j/src/main/scala/org/apache/pekko/event/slf4j/Slf4jLogger.scala#L200

Similar problem was reported in stackoverflow. Probably, scaladoc can not generate links for java external library.

@pjfanning Which solution do you think is better?:

  1. Ignore warn logs. Those links work well in Intellij IDEA.
  2. Use URL links (e.g. rewrite [[org.slf4j.Marker]] to [[https://javadoc.io/static/org.slf4j/slf4j-api/1.7.36/org/slf4j/Marker.html org.slf4j.Marker]]).
    • It can generate valid links in scaladoc, but developers can't jump to library sources in Intellij IDEA.
    • I think It has worse maintainability because of URL links contain library version.
pjfanning commented 1 year ago

@ydash Don't do 2.

Just do best effort to fix whatever scaladoc warnings are easily fixed.

I think there is a way to get the external links to slf4j to work. I've done it before but I don't have time today.

Here is an example from another project I work on.

https://github.com/FasterXML/jackson-module-scala/blob/2.16/build.sbt#L19-L36

ydash commented 1 year ago

I tried to add settings to Doc.scala like following, but (Compile / fullClasspath).value can not get subproject's jar paths in unidoc task.

https://github.com/apache/incubator-pekko/blob/88bf6329f193eedd45091f4f9a515943bd8ecb23/project/Doc.scala#L159

       Seq(
         ScalaUnidoc / unidocProjectFilter := unidocRootProjectFilter(unidocRootIgnoreProjects.value),
         JavaUnidoc / unidocProjectFilter := unidocRootProjectFilter(unidocRootIgnoreProjects.value),
-        ScalaUnidoc / apiMappings := (Compile / doc / apiMappings).value) ++
+        ScalaUnidoc / apiMappings := (Compile / doc / apiMappings).value,
+        ScalaUnidoc / apiMappings ++= {
+          def mappingsFor(organization: String, names: List[String], location: String, revision: (String) => String = identity): Seq[(File, URL)] =
+            for {
+              entry: Attributed[File] <- (Compile / fullClasspath).value
+              _ = println(s"entry: ${entry}")
+              module: ModuleID <- entry.get(moduleID.key)
+              if module.organization == organization
+              if names.exists(module.name.startsWith)
+            } yield entry.data -> url(location.format(revision(module.revision)))
+
+          val mappings: Seq[(File, URL)] =
+            mappingsFor("org.slf4j", List("slf4j-api"), "https://javadoc.io/doc/org.slf4j/slf4j-api/%s/")
+          mappings.toMap
+        }
+      ) ++
       UnidocRoot.CliOptions.genjavadocEnabled

@pjfanning Would you suggest any solutions?

pjfanning commented 1 year ago

@ydash I'm busy with release stuff so can't help much here.

I would have thought that Scala would handle api mappings for the various submodules of Pekko and that the only projects that we need to help Scala with are providing javadoc links for 3rd party libs like slf4j-api.

pjfanning commented 1 year ago

@ydash if you feel like it would be easier to start with more straightforward issues, maybe you could start with the doc issues where the Pekko class can't be found (because the doc reference doesn't include enough of the package name)

ydash commented 1 year ago

I submitted a PR(#477) which solves following problem:

https://github.com/apache/incubator-pekko/issues/353#issuecomment-1612738631

@pjfanning Could you see my PR?