Skip to content

Commit

Permalink
Add file path escaping to Scaladoc
Browse files Browse the repository at this point in the history
As Jekyll (and in extension GitHub pages) makes starting a file/folder
name with a couple characters illegal, an additional check is added for
those and problematic paths are reported.
To avoid always having "_docs" if not using -Yapi_subdirectory in url by
default in the static site, we also replace that with "docs".
Some tests concerning links were adjusted to accomodate the changes.
Example testcase for illegal jekyll chars in Scaladoc was also added.
  • Loading branch information
jchyb committed Mar 17, 2022
1 parent 17e46ad commit 4d6fd7e
Show file tree
Hide file tree
Showing 11 changed files with 58 additions and 15 deletions.
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package _docs.tests
package docs.tests

class Adoc:
def foo = 123
5 changes: 5 additions & 0 deletions scaladoc-testcases/src/example/JekyllIncompat.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package example

package jekyllIncompat:
object #~
object ~>~
17 changes: 17 additions & 0 deletions scaladoc/src/dotty/tools/scaladoc/DocContext.scala
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import java.io.PrintStream
import scala.io.Codec
import java.net.URL
import scala.util.Try
import scala.collection.mutable
import dotty.tools.scaladoc.util.Check._

type CompilerContext = dotty.tools.dotc.core.Contexts.Context

Expand Down Expand Up @@ -90,3 +92,18 @@ case class DocContext(args: Scaladoc.Args, compilerContext: CompilerContext):


val externalDocumentationLinks = args.externalMappings


// We collect and report any generated files incompatible with Jekyll
private lazy val jekyllIncompatLinks = mutable.HashSet[String]()

def checkPathCompat(path: Seq[String]): Unit =
if checkJekyllIncompatPath(path) then jekyllIncompatLinks.add(path.mkString("/"))

def reportPathCompatIssues(): Unit =
if !jekyllIncompatLinks.isEmpty then
report.warning(
s"""Following generated file paths might not be compatible with Jekyll:
|${jekyllIncompatLinks.mkString("\n")}
|If using GitHub Pages consider adding a \".nojekyll\" file.
""".stripMargin)(using compilerContext)
1 change: 1 addition & 0 deletions scaladoc/src/dotty/tools/scaladoc/Scaladoc.scala
Original file line number Diff line number Diff line change
Expand Up @@ -229,5 +229,6 @@ object Scaladoc:
given docContext: DocContext = new DocContext(args, ctx)
val module = ScalaModuleProvider.mkModule()
new dotty.tools.scaladoc.renderers.HtmlRenderer(module.rootPackage, module.members).render()
docContext.reportPathCompatIssues()
report.inform("generation completed successfully")
docContext
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ trait Locations(using ctx: DocContext):
case "<empty>" :: tail => "_empty_" :: tail
case other => other
if ctx.args.apiSubdirectory then "api" :: fqn else fqn
ctx.checkPathCompat(path)
cache.put(dri, path)
path
case cached => cached
Expand Down
2 changes: 1 addition & 1 deletion scaladoc/src/dotty/tools/scaladoc/renderers/Renderer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ abstract class Renderer(rootPackage: Member, val members: Map[DRI, Member], prot
val all = navigablePage +: redirectPages
// We need to check for conflicts only if we have top-level member called docs
val hasPotentialConflict =
rootPackage.members.exists(m => m.name.startsWith("_docs"))
rootPackage.members.exists(m => m.name.startsWith("docs"))

if hasPotentialConflict then
def walk(page: Page): Unit =
Expand Down
11 changes: 8 additions & 3 deletions scaladoc/src/dotty/tools/scaladoc/site/StaticSiteContext.scala
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,12 @@ class StaticSiteContext(
val docsPath = root.toPath.resolve("_docs")
val blogPath = root.toPath.resolve("_blog")

val relativizeFrom = if args.apiSubdirectory then docsPath else root.toPath
def relativize(path: Path): Path =
if args.apiSubdirectory then
docsPath.relativize(path)
else
val relativised = docsPath.relativize(path)
Paths.get("docs").resolve(relativised)

val siteExtensions = Set(".html", ".md")

Expand All @@ -47,7 +52,7 @@ class StaticSiteContext(
val redirectFrom = loadedTemplate.templateFile.settings.getOrElse("page", Map.empty).asInstanceOf[Map[String, Object]].get("redirectFrom")
def redirectToTemplate(redirectFrom: String) =
val path = if redirectFrom.startsWith("/")
then relativizeFrom.resolve(redirectFrom.drop(1))
then docsPath.resolve(redirectFrom.drop(1))
else loadedTemplate.file.toPath.resolveSibling(redirectFrom)
val driFrom = driFor(path)
val driTo = driFor(loadedTemplate.file.toPath)
Expand Down Expand Up @@ -93,7 +98,7 @@ class StaticSiteContext(
}

def driFor(dest: Path): DRI =
val rawFilePath = relativizeFrom.relativize(dest)
val rawFilePath = relativize(dest)
val pageName = dest.getFileName.toString
val dotIndex = pageName.lastIndexOf('.')

Expand Down
14 changes: 14 additions & 0 deletions scaladoc/src/dotty/tools/scaladoc/util/check.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package dotty.tools.scaladoc.util

object Check:
/**
* Jekyll (also used by GitHub Pages) by default makes a couple characters
* illegal to use in file name beginnings.
*/
def checkJekyllIncompatPath(path: Seq[String]): Boolean =
path.find( filename =>
filename.matches("^~.*")
|| filename.matches("^\\_.*")
|| filename.matches("^\\..*")
|| filename.matches("^\\#.*")
).isDefined
4 changes: 2 additions & 2 deletions scaladoc/test/dotty/tools/scaladoc/ReportingTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,11 @@ class ReportingTest:
val docsRoot = testDocPath.resolve("conflicts-pages").toString
checkReportedDiagnostics(_.copy(
docsRoot = Some(docsRoot),
tastyFiles = tastyFiles("tests", rootPck = "_docs")
tastyFiles = tastyFiles("tests", rootPck = "docs")
)){ diag =>
assertNoWarning(diag)
val Seq(msg) = diag.errorMsgs.map(_.toLowerCase)
Seq("conflict","api", "static", "page", "_docs/tests/adoc.html")
Seq("conflict","api", "static", "page", "docs/tests/adoc.html")
.foreach( word =>
Assert.assertTrue(s"Error message: $msg should contains $word", msg.contains(word))
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,5 @@ class NavigationTest extends BaseHtmlTest:
)),
))

testNavMenu("_docs/Adoc.html", topLevelNav)
testNavMenu("docs/Adoc.html", topLevelNav)
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ class SiteGeneratationTest extends BaseHtmlTest:
}

def testDocPages()(using ProjectContext) =
checkFile("_docs/Adoc.html")(title = "Adoc", header = "Header in Adoc", parents = Seq(projectName))
checkFile("_docs/dir/index.html")(title = "A directory", header = "A directory", parents = Seq(projectName))
checkFile("_docs/dir/nested.html")(
checkFile("docs/Adoc.html")(title = "Adoc", header = "Header in Adoc", parents = Seq(projectName))
checkFile("docs/dir/index.html")(title = "A directory", header = "A directory", parents = Seq(projectName))
checkFile("docs/dir/nested.html")(
title = "Nested in a directory", header = "Nested in a directory", parents = Seq(projectName, "A directory"))

def testDocIndexPage()(using ProjectContext) =
checkFile("_docs/index.html")(title = projectName, header = s"$projectName in header")
checkFile("docs/index.html")(title = projectName, header = s"$projectName in header")

def testApiPages(
mainTitle: String = "API",
Expand Down Expand Up @@ -66,13 +66,13 @@ class SiteGeneratationTest extends BaseHtmlTest:
testDocIndexPage()
testApiPages()

withHtmlFile("_docs/Adoc.html"){ content =>
withHtmlFile("docs/Adoc.html"){ content =>
content.assertAttr("p a","href", "../tests/site/SomeClass.html")
}

withHtmlFile("tests/site/SomeClass.html"){ content =>
content.assertAttr(".breadcrumbs a","href",
"../../_docs/index.html", "../../index.html", "../site.html", "SomeClass.html"
"../../docs/index.html", "../../index.html", "../site.html", "SomeClass.html"
)
}
}
Expand All @@ -98,7 +98,7 @@ class SiteGeneratationTest extends BaseHtmlTest:
@Test
def staticLinking() = withGeneratedSite(testDocPath.resolve("static-links")){

withHtmlFile("_docs/Adoc.html"){ content =>
withHtmlFile("docs/Adoc.html"){ content =>
content.assertAttr("p a","href",
"dir/html.html",
"dir/name...with..dots..html",
Expand Down

0 comments on commit 4d6fd7e

Please sign in to comment.