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 the API base URI to the Laika config #225

Open
wants to merge 2 commits into
base: series/0.4
Choose a base branch
from

Conversation

DavidGregory084
Copy link
Member

This enables the API Documentation Linking feature of Laika to work if API doc base URL is declared in the build config.

@armanbilge
Copy link
Member

armanbilge commented Mar 24, 2022

Aha, very nice! I was recently experimenting with this feature in http4s-dom.

I really want to do something like this too. There's a couple bits of trickyness:

  1. The tlSiteApiUrl is not necessarily the appropriate form of base URL for the Laika's API linking feature ... although this may have been a problem of my own making 😅 see:

    val p = tlSiteApiPackage.value.fold("")(_.replace('.', '/') + "/index.html")
    url(s"https://www.javadoc.io/doc/$o/$n/$v/$p")

    Practically, it's the difference between:

    In fact, I reckon if you try your change with the sbt-typelevel docs it'll have this problem. (This is the basic principle of sbt-typelevel: dog-food everything!)

  2. In user-land, if I want to add more packages to this apiLinks config, AFAICT it doesn't really compose well and I'd have to start from scratch. If we exposed this specific config as a sbt setting, this would make it easier.

    I'm not sure if this is a common enough problem to justify building the machinery for it, but it is helpful for http4s where the satellite repos tend to be implementations of APIs living in the core repo. Also it would be really cool to have this work out of the box for all cats(-effect) APIs etc.

WDYT?

@DavidGregory084
Copy link
Member Author

1. The `tlSiteApiUrl` is not necessarily the appropriate form of base URL for the Laika's API linking feature ... although this may have been a problem of my own making 😅 see:
   https://github.com/typelevel/sbt-typelevel/blob/f2db840015c44621f15607eb40b0cd9abbd9e829/site/src/main/scala/org/typelevel/sbt/TypelevelSitePlugin.scala#L137-L138
   
   Practically, it's the difference between:
   
   * https://www.javadoc.io/doc/org.typelevel/sbt-typelevel-docs_2.12/latest/index.html
   * https://www.javadoc.io/doc/org.typelevel/sbt-typelevel-docs_2.12/latest/org/typelevel/sbt/index.html
   
   In fact, I reckon if you try your change with the sbt-typelevel docs it'll have this problem. (This is the basic principle of sbt-typelevel: dog-food everything!)

Hmm yes I see what you mean; I was testing this with scalacache but I neglected to set the tlSiteApiPackage so everything looked correct. I suppose we could move the usage of tlSiteApiPackage out of the tlSiteApiUrl and into the code which creates the API doc links? It's still a nice usability feature.

2. In user-land, if I want to add more packages to this `apiLinks` config, AFAICT it doesn't really compose well and I'd have to start from scratch. If we exposed this specific config as a sbt setting, this would make it easier.
   I'm not sure if this is a common enough problem to justify building the machinery for it, but it is helpful for http4s where the satellite repos tend to be implementations of APIs living in the core repo. Also it would be really cool to have this work out of the box for all cats(-effect) APIs etc.

Yeah it seems the Laika config is a little tricky to work with in general, although I think that withConfigValue might be additive as it seems updates an underlying builder object? It's a shame that =~ doesn't work well with .value anymore as it would make this kind of thing easier to use.

laikaConfig := {
val currentConfig = laikaConfig.value
tlSiteApiUrl.value.fold(currentConfig) { apiUrl =>
currentConfig.withConfigValue(LinkConfig(apiLinks = Seq(ApiLinks(apiUrl.toString))))
Copy link
Member

Choose a reason for hiding this comment

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

Following on the additivity issue, .withConfigValue is definitely additive. But I assume that this will override any previously set LinkConfig and esp. all apiLinks previously set?

Copy link
Member Author

@DavidGregory084 DavidGregory084 Mar 24, 2022

Choose a reason for hiding this comment

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

I guess only way is to test it!

scala> import laika.sbt._
import laika.sbt._

scala> import laika.rewrite.link._
import laika.rewrite.link._

scala> val conf = LaikaConfig.defaults
conf: laika.sbt.LaikaConfig = LaikaConfig(UTF-8,<function1>,laika.config.ConfigBuilder@302e25d3,laika.ast.MessageFilter$$anon$1@78ea076,None,laika.ast.MessageFilter$$anon$1@56276b5d)

scala> val confWithLinks = conf.withConfigValue(LinkConfig(apiLinks = List(ApiLinks(baseUri = "https://www.javadoc.io/doc/com.github.cb372/scalacache-unidocs_2.13/0.28.0/"))))
confWithLinks: laika.sbt.LaikaConfig = LaikaConfig(UTF-8,<function1>,laika.config.ConfigBuilder@623f79d4,laika.ast.MessageFilter$$anon$1@78ea076,None,laika.ast.MessageFilter$$anon$1@56276b5d)
                                                                                                                                                                                                           
scala> val confWithMoreLinks = confWithLinks.withConfigValue(LinkConfig(apiLinks = List(ApiLinks(baseUri = "https://www.javadoc.io/doc/org.typelevel/sbt-typelevel-docs_2.12/latest/", "org.typelevel.sbt"))))
confWithMoreLinks: laika.sbt.LaikaConfig = LaikaConfig(UTF-8,<function1>,laika.config.ConfigBuilder@1698fd02,laika.ast.MessageFilter$$anon$1@78ea076,None,laika.ast.MessageFilter$$anon$1@56276b5d)

scala> confWithLinks.configBuilder.build.get[LinkConfig]("laika.links")
res0: laika.config.Config.ConfigResult[laika.rewrite.link.LinkConfig] = Right(LinkConfig(List(),List(),List(ApiLinks(https://www.javadoc.io/doc/com.github.cb372/scalacache-unidocs_2.13/0.28.0/,*,index.html)),List()))

scala> confWithMoreLinks.configBuilder.build.get[LinkConfig]("laika.links")
res1: laika.config.Config.ConfigResult[laika.rewrite.link.LinkConfig] = Right(LinkConfig(List(),List(),List(ApiLinks(https://www.javadoc.io/doc/org.typelevel/sbt-typelevel-docs_2.12/latest/,org.typelevel.sbt,index.html)),List()))

scala>

It looks like yes, latest ApiLinks wins 😬

@armanbilge
Copy link
Member

armanbilge commented Mar 24, 2022

I suppose we could move the usage of tlSiteApiPackage out of the tlSiteApiUrl and into the code which creates the API doc links? It's still a nice usability feature.

Yeah, although it's less about the usage of tlSiteApiPackage and more that tlSiteApiUrl currently means "what the API docs button in the website links to" and not "a machine-readable API url". Presumably anyone can set tlSiteApiUrl to anything :)

Yeah it seems the Laika config is a little tricky to work with in general

Yeppp. For example, right now I don't think it's possible to modify the config to replace/remove the Typelevel logo and social links at the top without losing the API and github links as well. This is unfortunate for the http4s plugin which has to replicate this functionality and for anyone outside Typelevel that wants to enjoy the goodies in this plugin.

https://github.com/http4s/sbt-http4s-org/blob/e2c730ccedc3fd13495fe2ea0c38eaad2e4bbdce/core/src/main/scala/org/http4s/sbt/Http4sOrgSitePlugin.scala#L93

So, our recourse is to expose all these things in sbt settings, as I'm tempted to here. Then we can auto-populate it with useful stuff (like the projects own API docs) while letting downstreams add whatever else they might need.

Edit: I guess the upstream fix is for Laika to expose the current config in the builder, if possible. But working with nested case classes starts needing lenses and the usability starts to get tricky.

@@ -113,7 +125,7 @@ object TypelevelSitePlugin extends AutoPlugin {
"PRERELEASE_VERSION" -> currentPreRelease.value.getOrElse(version.value),
"SNAPSHOT_VERSION" -> version.value
) ++
tlSiteApiUrl.value.map("API_URL" -> _.toString).toMap
tlSiteApiIndexUrl.value.map("API_URL" -> _.toString).toMap
Copy link
Member Author

Choose a reason for hiding this comment

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

I have tried adding a new setting to capture the distinction between the index page + the API base URL, but I don't love that these names are now out of sync

Copy link
Member

@armanbilge armanbilge Mar 24, 2022

Choose a reason for hiding this comment

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

Right. I think instead we should add a new setting tlSiteApiLinks that's maybe like Map[String -> url] package name to API docs url. Then we can make a best effort to populate this.

@mergify mergify bot added the site label Jun 4, 2022
@armanbilge
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants