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

jchyb · 2022-03-10T10:44:27Z

This PR adds functionality of escaping special characters in generated filenames. These rules consist of the default Jekyll rules, which do not allow to put some chars in certain places of names of the deployed files - otherwise those file are not being generated. Since GitHub Pages uses Jekyll for deployment, an effect of these rules can be wide (even if the Jekyll itself can have those rules changed).
Those rules can be found here.
~~They consist of:~~
* _ at the beginning of the filenames
* ~ at the beginning of the filenames (docs above mention the end of the filenames, but neither the original issue nor my tests confirm)
* # at the beginning of the filenames
* . at the beginning of the filenames (doesn’t really matter here, but added for clarity and to future-proof)

Tests were adjusted to accomodate the changes.
I also tested GitHub Pages manually: without the PR, with the PR.

Instead of escaping the characters, we collect them and report them in a warning after generating the documentation, hinting about adding a .nojekyll file if using GitHub Pages.

Important: will break links (previous _docs will become docs if -Yapi-subdirectory is not used).

Fixes #14612

julienrf · 2022-03-16T08:11:35Z

If the problem manifests only with Jekyll, I’d be tempted to find a fix on the Jekyll side rather than the Scaladoc side.

What happens if you do .nojekyll?

Otherwise, could you please provide a list of page URLs in the Scala API that are affected by this change?

jchyb · 2022-03-16T09:27:13Z

Yes, .nojekyll would fix it, I imagine. Same with the include option in the .config.yml file. What made me provide the fix was the idea that Scala 2 Scaladoc handles cases like this by default (as mentioned in #14612), but otherwise I have got no strong opinions about this. I realize that breaking linking compatibility is always problematic. In Scala 3 api I only found api/scala/%23::$.html that would become api/scala/$hash::$.html

pikinier20 · 2022-03-16T13:37:59Z

I'd say we need to merge that before 3.1.3-RC1 release because otherwise users would need to work with Jekyll to have their static sites deployed.

julienrf · 2022-03-16T14:11:28Z

We need to make a change in static site to make it generate under docs instead of _docs.

IIUC, the output of Scaladoc does not contain _docs? (unless you name your package _docs, but this is the responsibility of the programmer, not of Scaladoc) So, that would not be a problem to be deployed on GitHub Pages?

pikinier20 · 2022-03-16T14:38:49Z

We need to make a change in static site to make it generate under docs instead of _docs.

IIUC, the output of Scaladoc does not contain _docs? (unless you name your package _docs, but this is the responsibility of the programmer, not of Scaladoc) So, that would not be a problem to be deployed on GitHub Pages?

It does contain _docs when you create static site without -Yapi-subdirectory

pikinier20

There's only one thing to change here. Besides that, LGTM.

pikinier20 · 2022-03-16T14:42:06Z

scaladoc/src/dotty/tools/scaladoc/site/StaticSiteContext.scala

+      val relativised = root.toPath.relativize(path).toString()
+      // we remove all the '_' from _docs, _blog ... as having
+      // $underlinedocs in the url by default will look quite ugly
+      val withUnderlineRemoved = relativised.replaceFirst("^\\_", "")
+      Paths.get(withUnderlineRemoved)


We should there just do:

root.toPath.resolve("docs").relativize(path)

julienrf · 2022-03-16T14:46:49Z

It does contain _docs when you create static site without -Yapi-subdirectory

Oh, that’s an issue then. I thought _docs was only the name of the source directory, not of the output directory.

pikinier20 · 2022-03-16T14:48:28Z

If the problem manifests only with Jekyll, I’d be tempted to find a fix on the Jekyll side rather than the Scaladoc side.

What happens if you do .nojekyll?

Otherwise, could you please provide a list of page URLs in the Scala API that are affected by this change?

I agree that when user decides to create a page which is not accepted by Jekyll then they should take care of that in Jekyll.

In case of the _docs the problem is on our side since user can't do anything to change it.

julienrf · 2022-03-16T15:02:54Z

scaladoc/src/dotty/tools/scaladoc/renderers/Renderer.scala

@@ -101,7 +101,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"))


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

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.

julienrf · 2022-03-16T15:05:00Z

In case of the _docs the problem is on our side since user can't do anything to change it.

I agree with changing this in Scaladoc too. I am not sure about escaping some symbols because of Jekyll. I prefer if we tell the users to put a .nojekyll file.

pikinier20 · 2022-03-16T15:30:45Z

In case of the _docs the problem is on our side since user can't do anything to change it.

I agree with changing this in Scaladoc too. I am not sure about escaping some symbols because of Jekyll. I prefer if we tell the users to put a .nojekyll file.

We can check generated links if they contain invalid characters and produce a warning with possible solution. Should we have a -Yjekyll-friendly option to escape characters?

julienrf · 2022-03-16T15:35:56Z

We can check generated links if they contain invalid characters and produce a warning with possible solution.

I like this idea! I prefer not introducing another configuration flag 😄

jchyb · 2022-03-16T16:15:45Z

Sound good to me as well. I will redo this PR so that we:

replace the _docs with docs like it is already being done here
throw warning with all the problematic links collected there (could be good if for whatever reason users will still want to use jekyll, then they can easily configure the jekyll config file themselves manually excluding the files) with a suggestion of adding a .nojekyll file if using Github Pages
maybe add a general warning about jekyll in the Scala 3 scaladoc documentation

jchyb · 2022-03-17T09:56:01Z

scaladoc/src/dotty/tools/scaladoc/site/StaticSiteContext.scala

-  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 = root.toPath.relativize(path).toString()
+      // we remove all the '_' from _docs to avoid unnecessary
+      // incompatiblilites with Jekyll
+      val withUnderlineRemoved = relativised.replaceFirst("^\\_", "")
+      Paths.get(withUnderlineRemoved)


Unfortunately I was not able to simplify this, @pikinier20 suggestion resulted in obtaining paths like ../_docs/A.md instead of docs/A.md like we want to.

Could we then just be specific to _docs?

ie, relativised.replaceFirst("^\\_docs", "docs") (it’s a pity that there is no replaceFirstLiterally)

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

julienrf · 2022-03-17T12:11:18Z

scaladoc/src/dotty/tools/scaladoc/DocContext.scala

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


What is the scope of this cache?

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

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.

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?

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

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.

pikinier20 · 2022-03-17T15:08:09Z

scaladoc/src/dotty/tools/scaladoc/DocContext.scala

@@ -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._


Where is this import used?

Now I've made the import more explicit since it was only one method, not sure why I made it like this before

seems like nothing changed here.
There were problems with GitHub last week. Perhaps something didn't get pushed

That's true, thanks for noticing, I've force pushed it just now.

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.

jchyb added the area:doctool label Mar 10, 2022

jchyb requested a review from pikinier20 March 10, 2022 10:44

jchyb force-pushed the scaladoc/jekyll-escapes branch from cce9982 to 1c4489c Compare March 16, 2022 01:32

pikinier20 suggested changes Mar 16, 2022

View reviewed changes

julienrf reviewed Mar 16, 2022

View reviewed changes

jchyb force-pushed the scaladoc/jekyll-escapes branch from 1c4489c to 4402151 Compare March 17, 2022 09:08

jchyb commented Mar 17, 2022

View reviewed changes

jchyb changed the title ~~Scaladoc: Escape special characters in filenames according to the default Jekyll rules~~ Scaladoc: Warn about special characters in filenames according to the default Jekyll rules Mar 17, 2022

jchyb force-pushed the scaladoc/jekyll-escapes branch from 4402151 to 60a0ed7 Compare March 17, 2022 11:40

julienrf reviewed Mar 17, 2022

View reviewed changes

jchyb force-pushed the scaladoc/jekyll-escapes branch from 60a0ed7 to 4d6fd7e Compare March 17, 2022 14:16

pikinier20 reviewed Mar 17, 2022

View reviewed changes

jchyb force-pushed the scaladoc/jekyll-escapes branch from 4d6fd7e to b8ffef5 Compare March 21, 2022 10:44

pikinier20 approved these changes Mar 25, 2022

View reviewed changes

julienrf approved these changes Mar 25, 2022

View reviewed changes

pikinier20 force-pushed the scaladoc/jekyll-escapes branch from b8ffef5 to d3ce129 Compare April 25, 2022 07:05

pikinier20 enabled auto-merge April 25, 2022 07:05

pikinier20 merged commit 4e41a4c into scala:main Apr 25, 2022

Kordyjan added this to the 3.2.0 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jchyb commented Mar 10, 2022 •

edited

Loading

julienrf commented Mar 16, 2022

jchyb commented Mar 16, 2022

pikinier20 commented Mar 16, 2022

julienrf commented Mar 16, 2022

pikinier20 commented Mar 16, 2022

pikinier20 left a comment

pikinier20 Mar 16, 2022

julienrf commented Mar 16, 2022

pikinier20 commented Mar 16, 2022

julienrf Mar 16, 2022

pikinier20 Mar 16, 2022

julienrf commented Mar 16, 2022

pikinier20 commented Mar 16, 2022

julienrf commented Mar 16, 2022

jchyb commented Mar 16, 2022

jchyb Mar 17, 2022

julienrf Mar 17, 2022

pikinier20 Mar 17, 2022 •

edited

Loading

julienrf Mar 17, 2022

jchyb Mar 17, 2022

pikinier20 Mar 17, 2022

julienrf Mar 17, 2022

jchyb Mar 24, 2022

pikinier20 Mar 25, 2022

pikinier20 Mar 17, 2022

jchyb Mar 17, 2022

pikinier20 Mar 21, 2022

jchyb Mar 21, 2022



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

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

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

Conversation

jchyb commented Mar 10, 2022 • edited Loading

julienrf commented Mar 16, 2022

jchyb commented Mar 16, 2022

pikinier20 commented Mar 16, 2022

julienrf commented Mar 16, 2022

pikinier20 commented Mar 16, 2022

pikinier20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrf commented Mar 16, 2022

pikinier20 commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrf commented Mar 16, 2022

pikinier20 commented Mar 16, 2022

julienrf commented Mar 16, 2022

jchyb commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pikinier20 Mar 17, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchyb commented Mar 10, 2022 •

edited

Loading

pikinier20 Mar 17, 2022 •

edited

Loading