Skip to content

Scaladoc: Warn about special characters in filenames according to the default Jekyll rules #14657

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 25, 2022
Merged

Scaladoc: Warn about special characters in filenames according to the default Jekyll rules #14657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...adoc-testcases/src/_docs/tests/Adoc.scala → scaladoc-testcases/src/docs/tests/Adoc.scala
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.checkJekyllIncompatPath

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]()
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the scope of this cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used anywhere where Paths are generated, from Tasty reading to rendering, so DocContext felt like the right place for it

Copy link
Contributor

Choose a reason for hiding this comment

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

One problem that might come is that this set exists throughout the life of the entire Scaladoc process. However, it shouldn't be that big so that's probably OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t think it would be too big. My concern is that if we run scaladoc twice it may still warn about the files of the first run, or something like that.
Would it be possible to emit the warnings on the fly, rather than keeping a global mutable set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late response. It would be possible to emit them on the fly, but it would be much less elegant in my opinion with harder to read repeated suggestions. With any empty package potentially causing multiple paths to be affected, users are going to see this warning a lot, so I would personally prefer it to be as concise as possible - also having the paths printed right next to each other will make them easier to work with in for the Jekyll config. I'm unsure about the scope now - I was under the impression that the scaladoc process always ended after generation, but I don't actually know how it's handled by sbt etc. @pikinier20 What do you think? Sorry for dragging this issue for such a long time

Copy link
Contributor

Choose a reason for hiding this comment

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

The DocContext is re-created on each run so we dont need to worry about warnings from previous runs. Also, I don't have strong opinion about keeping links in set instead of warning on the fly. For me, it can stay as it is.


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
View file
Open in desktop
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
1 change: 1 addition & 0 deletions scaladoc/src/dotty/tools/scaladoc/renderers/Locations.scala
View file
Open in desktop
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
View file
Open in desktop
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"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn’t it the case only if apiSubdirectory is false? Also, would a package named blog cause a conflict too?

Copy link
Contributor

Choose a reason for hiding this comment

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

blog won't cause a conflict because it's rendered under docs

This place is just a heuristics that informs user about potential conflict. Even so, of course, checking the apiSubdirectory flag seems sensible.


if hasPotentialConflict then
def walk(page: Page): Unit =
Expand Down
11 changes: 8 additions & 3 deletions scaladoc/src/dotty/tools/scaladoc/site/StaticSiteContext.scala
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion scaladoc/test/dotty/tools/scaladoc/site/NavigationTest.scala
View file
Open in desktop
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)
}
14 changes: 7 additions & 7 deletions scaladoc/test/dotty/tools/scaladoc/site/SiteGeneratationTest.scala
View file
Open in desktop
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