Refactor: Improve error messaging in sidebar YAML parser #17229

Dedelweiss · 2023-04-11T11:51:51Z

Validations

On this PR I added some errors for different conditions :

Error when a path in page is wrong
Error when an index or a page path for a title is missing
(It is therefore activated with the title.isEmpty condition. So far I've only found that when a user forgets a title it gives r = RawInput(,,,[],,false) in the toSidebar function. Thus, title.isEmpty is necessarily empty. However, if another error produces an r = RawInput(,,[],,false). This condition and its error message will no longer make sense. I am therefore less sure about this condition.)
Error when a title for a page is missing
Warning when something is wrong but it does not interfere with the generation.

Tests

I added two functions in SidebarParserTest.
The purpose of these functions is to check if it contains the schemaMessage which is the general template sent in all error messages + the unique error message for each error.

IE

I also noticed that @gringrape wanted to work on this issue, if a code has already been done on his side, I am quite open to resume the work started or to close my PR to leave his. I am also open to any suggestions for adding errors.

Fixes: #15326

julienrf · 2023-04-12T07:58:15Z

Hey @Dedelweiss, thank you very much for working on this!

About the first point: check the code that calls Sidebar.toSidebar, at some point you should get the root directory of the documentation files. Then you have two options: either pass the root directory to Sidebar.toSidebar and perform the check that the files exist here, or perform the check in the part of the code that calls Sidebar.toSidebar (that part of the code should have both the documentation root directory and the parsed sidebar). Does that make sense? Note that that could be achieved in a separate PR, if you want to keep things simple.

Dedelweiss · 2023-04-12T10:42:38Z

About the first point: check the code that calls Sidebar.toSidebar, at some point you should get the root directory of the documentation files. Then you have two options: either pass the root directory to Sidebar.toSidebar and perform the check that the files exist here

Hello @julienrf ! Thank you very much, it works perfectly.

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

Florian3k

Looks good, I left one comment.

julienrf

Is there a possibility to create tests that cover this feature?

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

Florian3k

Sorry, I just realized there are some problems with validation logic. I will get back to you with more precise information soon.

Dedelweiss · 2023-05-05T08:06:48Z

Yes, I'm currently working on it, sorry.

Dedelweiss · 2023-05-09T14:49:20Z

Edited: Just Added a new method to correct this. 👍

Hi, I'm working on the test and fixing the problem of the error message when the sidebar is correctly formatted.
I have sent a first commit for this. In this commit I changed the last one to a warning. Because even the static site is generated, there is a problem in the generation. For example, when you put :

- title: myFoo
   page: page1
   page: page2

Now I'm not sure if I missed any errors and I'm testing this.
For the test, I haven't finished the one where the title is missing. Because I don't get a single error message like :
report.error(s"$msg\n$schemaMessage")
But several errors and an exception like this:

Static site YAML configuration file should comply with the following description:
The root element of static site needs to be <subsection>
`title` and `directory` properties are ignored in root subsection.

<subsection>:
  title: <string> # optional - Default value is file name. Title can be also set using front-matter.
  index: <string> # optional - If not provided, default empty index template is generated.
  directory: <string> # optional - By default, directory name is title name in kebab case.
  subsection: # optional - If not provided, pages are loaded from the index directory
    - <subsection> | <page>
  # either index or subsection needs to be present
<page>:
  title: <string> # optional - Default value is file name. Title can be also set using front-matter.
  page: <string>
  hidden: <boolean> # optional - Default value is false.

For more information visit:
https://docs.scala-lang.org/scala3/guides/scaladoc/static-site.html: com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot deserialize value of type `java.util.ArrayList<dotty.tools.scaladoc.site.Sidebar$RawInput>` from Object value (token `JsonToken.START_OBJECT`)
 at [Source: (StringReader); line: 3, column: 5] (through reference chain: dotty.tools.scaladoc.site.Sidebar$RawInput["subsection"])
        at com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:59)
        at com.fasterxml.jackson.databind.DeserializationContext.reportInputMismatch(DeserializationContext.java:1741)
        at com.fasterxml.jackson.databind.DeserializationContext.handleUnexpectedToken(DeserializationContext.java:1515)
Error parsing YAML configuration file: Title is not provided.
Static site YAML configuration file should comply with the following description:
The root element of static site needs to be <subsection>
`title` and `directory` properties are ignored in root subsection.

<subsection>:
  title: <string> # optional - Default value is file name. Title can be also set using front-matter.
  index: <string> # optional - If not provided, default empty index template is generated.
  directory: <string> # optional - By default, directory name is title name in kebab case.
  subsection: # optional - If not provided, pages are loaded from the index directory
    - <subsection> | <page>
  # either index or subsection needs to be present
<page>:
  title: <string> # optional - Default value is file name. Title can be also set using front-matter.
  page: <string>
  hidden: <boolean> # optional - Default value is false.

For more information visit:
https://docs.scala-lang.org/scala3/guides/scaladoc/static-site.html
Root element is not a subsection.
Static site YAML configuration file should comply with the following description:
The root element of static site needs to be <subsection>
`title` and `directory` properties are ignored in root subsection.

<subsection>:
  title: <string> # optional - Default value is file name. Title can be also set using front-matter.
  index: <string> # optional - If not provided, default empty index template is generated.
  directory: <string> # optional - By default, directory name is title name in kebab case.
  subsection: # optional - If not provided, pages are loaded from the index directory
    - <subsection> | <page>
  # either index or subsection needs to be present
<page>:
  title: <string> # optional - Default value is file name. Title can be also set using front-matter.
  page: <string>
  hidden: <boolean> # optional - Default value is false.

For more information visit:
https://docs.scala-lang.org/scala3/guides/scaladoc/static-site.html

I think because the title is missing, it goes to .fold and then to case _ in the toSidebar match. So I'm looking for a way to get a single error message.

julienrf

Thank you for adding tests @Dedelweiss! I have left a couple of comments.

scaladoc/test/dotty/tools/scaladoc/site/SidebarParserTest.scala

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

Florian3k · 2023-05-12T08:23:49Z

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

    case RawInput(title, page, index, subsection, dir, hidden) =>
-      report.error(s"Error parsing YAML configuration file.\n$schemaMessage")
+      if title.isEmpty() then


Why are we checking this? title is optional

So it's optional for subsection, - page but not for a simple page from what I have seen in my tests. Example:

- title: My title page: my-page1.md

But I noticed that it works with an index before (it's just ignored), so I'm thinking of adding && index.isEmpty.

- index: my-page2.md page: my-page1.md

and this one works too:

index: index.md subsection: - page: my-page1.md page: my-page2.md - page: my-page3.md

But the pages rendered are my-page2.md & my-page3.md

In this commit: - Added a error message when the path of a page is wrong - Added an error message when an inddex or a page path in a subsection is missing - Added an error message when a title in a subsection is missing

- Change the case match (Overkill) to a if else - Two types of messages : - Error: NoTitle and No index or No page. - Warning: When the parsing is not done correctly. Example: When a title got two pages.

- Error when a title but no pages

- Do a new method of test (Use a 'contains' instead) - Improve the accuracy of the error message by putting variables.

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

julienrf · 2023-05-25T12:55:02Z

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

+        report.error(s"$msg\n$schemaMessage")
+      else
+        val msg = s"Error parsing YAML configuration file."
+        report.warning(s"$msg\n$schemaMessage")
      Sidebar.Page(None, page, hidden)


Why do we still return a Sidebar even though we reported errors?

Indeed, I think there's a problem with this return. Because we need to return the Sidebar type but with Sidebar.Page(None, page, hidden). When a user forgets a page associated with a title like :

index: index.md subsection : - title : My title - page: my-page2.md

This sends an error but also blocks the generation of all the documentation (which is rather extreme).
See if I can return Sidebar.Category(None, None, Nil, None) instead.

julienrf · 2023-05-25T12:57:26Z

Now, I don't know if it's better to let an else in case I missed a possible error and the user still gets an error message.
If you know of any other possible errors or have any feedback I am very interested.

How is it that you know the sidebar is not valid, but you don’t know what is invalid?

Dedelweiss · 2023-05-26T09:32:30Z

How is it that you know the sidebar is not valid, but you don’t know what is invalid?

So, I think I've tested all the possible errors with a minimised code but I thought that the security of sending an error if a user didn't pass one of my conditions wouldn't be bad.

julienrf

Thank you Lucas!

ckipp01

Just a small nit.

ckipp01 · 2023-06-08T13:11:56Z

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

    case RawInput(title, page, index, subsection, dir, hidden) =>
-      report.error(s"Error parsing YAML configuration file.\n$schemaMessage")
+      if title.isEmpty() && index.isEmpty() then
+        val msg = "`title` property is missing for some page."


Since we have access to page up above here, can't we specify which page is actually missing the title instead of just saying "some page"?

From what I understood this is not possible, see #17229 (comment)

ckipp01

Just a small nit.

julienrf · 2023-06-12T08:45:56Z

It seems the tests added in this PR fail on Windows, see https://github.com/lampepfl/dotty/actions/runs/5240593619/jobs/9461621127. @Dedelweiss, would you have time to look at the failures?

Dedelweiss · 2023-06-12T09:28:41Z

It seems the tests added in this PR fail on Windows, see https://github.com/lampepfl/dotty/actions/runs/5240593619/jobs/9461621127. @Dedelweiss, would you have time to look at the failures?

Hello Julien, sure, I will look at it.

The purpose of this Pull request is to correct my PR #17229 Sorry for this failure. I have tested the following command locally and my tests pass. `sbt ";dist/pack ;scala3-bootstrapped/compile ;scala3-bootstrapped/test"` [test_windows_full] Fixes: #17963

The goal of this PR is to re-add the path validation of the page in a sidebar.yaml and improve the Testcase in the `SidebarParserTest` by erasing the first title, so it's easier to understand the problem in it. This PR is related to PRs: #17229 & #17959 [test_windows_full] Co-authored-by: Lucas Leblanc <[email protected]>

Fixes: #15326 [Cherry-picked daa9934]

The purpose of this Pull request is to correct my PR #17229 Sorry for this failure. I have tested the following command locally and my tests pass. `sbt ";dist/pack ;scala3-bootstrapped/compile ;scala3-bootstrapped/test"` [test_windows_full] Fixes: #17963 [Cherry-picked d83aa49]

…o LTS (#18982) Backports #17229 to the LTS branch. PR submitted by the release tooling. [skip ci]

The goal of this PR is to re-add the path validation of the page in a sidebar.yaml and improve the Testcase in the `SidebarParserTest` by erasing the first title, so it's easier to understand the problem in it. This PR is related to PRs: #17229 & #17959 [test_windows_full] Co-authored-by: Lucas Leblanc <[email protected]> [Cherry-picked cf50fe3]

Dedelweiss force-pushed the error_sidebar branch from 993869b to ec6174e Compare April 11, 2023 13:21

Dedelweiss force-pushed the error_sidebar branch from ec6174e to 80bfcd7 Compare April 12, 2023 09:36

Dedelweiss marked this pull request as ready for review April 12, 2023 13:07

Dedelweiss changed the title ~~Improve error messaging in sidebar YAML parser~~ Feat: Improve error messaging in sidebar YAML parser Apr 12, 2023

anatoliykmetyuk requested a review from pikinier20 April 17, 2023 12:08

anatoliykmetyuk assigned pikinier20 Apr 17, 2023

KacperFKorban assigned Florian3k and unassigned pikinier20 Apr 19, 2023

KacperFKorban requested review from Florian3k and removed request for pikinier20 April 19, 2023 22:09

Florian3k reviewed May 4, 2023

View reviewed changes

scaladoc/src/dotty/tools/scaladoc/site/SidebarParser.scala Outdated Show resolved Hide resolved

Florian3k approved these changes May 4, 2023

View reviewed changes

julienrf reviewed May 5, 2023

View reviewed changes

scaladoc/src/dotty/tools/scaladoc/site/SidebarParser.scala Outdated Show resolved Hide resolved

Florian3k self-requested a review May 5, 2023 07:44

Florian3k requested changes May 5, 2023

View reviewed changes

Dedelweiss force-pushed the error_sidebar branch 2 times, most recently from fd43076 to 4a668a8 Compare May 8, 2023 13:06

Dedelweiss requested review from Florian3k and julienrf May 11, 2023 12:12

julienrf reviewed May 11, 2023

View reviewed changes

scaladoc/test/dotty/tools/scaladoc/site/SidebarParserTest.scala Outdated Show resolved Hide resolved

scaladoc/src/dotty/tools/scaladoc/site/SidebarParser.scala Outdated Show resolved Hide resolved

Florian3k reviewed May 12, 2023

View reviewed changes

Dedelweiss added 5 commits May 24, 2023 14:09

Improve error messaging in sidebar YAML parser

b8d5207

In this commit: - Added a error message when the path of a page is wrong - Added an error message when an inddex or a page path in a subsection is missing - Added an error message when a title in a subsection is missing

Corrections:

dd5ebcc

- Change the case match (Overkill) to a if else - Two types of messages : - Error: NoTitle and No index or No page. - Warning: When the parsing is not done correctly. Example: When a title got two pages.

Add tests

3ce49f8

Add a first test:

cfa4461

- Error when a title but no pages

Corrections:

e9719f6

- Do a new method of test (Use a 'contains' instead) - Improve the accuracy of the error message by putting variables.

julienrf reviewed May 25, 2023

View reviewed changes

scaladoc/src/dotty/tools/scaladoc/site/SidebarParser.scala Outdated Show resolved Hide resolved

scaladoc/src/dotty/tools/scaladoc/site/SidebarParser.scala Outdated Show resolved Hide resolved

julienrf reviewed May 25, 2023

View reviewed changes

scaladoc/src/dotty/tools/scaladoc/site/SidebarParser.scala Outdated Show resolved Hide resolved

julienrf reviewed May 25, 2023

View reviewed changes

Avoid re-parse and change errors messages

5e7e205

julienrf approved these changes May 30, 2023

View reviewed changes

Florian3k approved these changes Jun 7, 2023

View reviewed changes

Dedelweiss changed the title ~~Feat: Improve error messaging in sidebar YAML parser~~ Refactor: Improve error messaging in sidebar YAML parser Jun 8, 2023

ckipp01 reviewed Jun 8, 2023

View reviewed changes

julienrf merged commit daa9934 into scala:main Jun 12, 2023

This was referenced Jun 12, 2023

Fix: Corret the PR17229 with the tests failing #17958

Closed

Fix: Error Sidebar PR failed test #17959

Merged

ckipp01 mentioned this pull request Jun 13, 2023

Nightly Dotty workflow of 2023-06-13 failed #17963

Closed

sjrd mentioned this pull request Jun 13, 2023

Remove OldTastyInspector from scaladoc #17751

Merged

Dedelweiss mentioned this pull request Jun 21, 2023

Refactor: Path validation and SiderbarParserTest #18000

Merged

Kordyjan added this to the 3.4.0 milestone Aug 2, 2023

Kordyjan mentioned this pull request Nov 17, 2023

Backport "Refactor: Improve error messaging in sidebar YAML parser" to LTS #18982

Merged

Kordyjan pushed a commit that referenced this pull request Nov 17, 2023

Refactor: Improve error messaging in sidebar YAML parser (#17229)

7c64ac6

Fixes: #15326 [Cherry-picked daa9934]

Kordyjan added a commit that referenced this pull request Nov 21, 2023

Backport "Refactor: Improve error messaging in sidebar YAML parser" t…

ec39c48

…o LTS (#18982) Backports #17229 to the LTS branch. PR submitted by the release tooling. [skip ci]

Kordyjan modified the milestones: 3.4.0, 3.3.2 Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Improve error messaging in sidebar YAML parser #17229

Refactor: Improve error messaging in sidebar YAML parser #17229

Dedelweiss commented Apr 11, 2023 •

edited

Loading

julienrf commented Apr 12, 2023 •

edited

Loading

Dedelweiss commented Apr 12, 2023

Florian3k left a comment

julienrf left a comment

Florian3k left a comment

Dedelweiss commented May 5, 2023 •

edited

Loading

Dedelweiss commented May 9, 2023 •

edited

Loading

julienrf left a comment

Florian3k May 12, 2023

Dedelweiss May 12, 2023

Dedelweiss May 12, 2023 •

edited

Loading

julienrf May 25, 2023

Dedelweiss May 26, 2023

julienrf commented May 25, 2023

Dedelweiss commented May 26, 2023

julienrf left a comment

ckipp01 left a comment

ckipp01 Jun 8, 2023

julienrf Jun 12, 2023

ckipp01 left a comment

julienrf commented Jun 12, 2023

Dedelweiss commented Jun 12, 2023

Refactor: Improve error messaging in sidebar YAML parser #17229

Refactor: Improve error messaging in sidebar YAML parser #17229

Conversation

Dedelweiss commented Apr 11, 2023 • edited Loading

Validations

Tests

IE

julienrf commented Apr 12, 2023 • edited Loading

Dedelweiss commented Apr 12, 2023

Florian3k left a comment

Choose a reason for hiding this comment

julienrf left a comment

Choose a reason for hiding this comment

Florian3k left a comment

Choose a reason for hiding this comment

Dedelweiss commented May 5, 2023 • edited Loading

Dedelweiss commented May 9, 2023 • edited Loading

julienrf left a comment

Choose a reason for hiding this comment

Florian3k May 12, 2023

Choose a reason for hiding this comment

Dedelweiss May 12, 2023

Choose a reason for hiding this comment

Dedelweiss May 12, 2023 • edited Loading

Choose a reason for hiding this comment

julienrf May 25, 2023

Choose a reason for hiding this comment

Dedelweiss May 26, 2023

Choose a reason for hiding this comment

julienrf commented May 25, 2023

Dedelweiss commented May 26, 2023

julienrf left a comment

Choose a reason for hiding this comment

ckipp01 left a comment

Choose a reason for hiding this comment

ckipp01 Jun 8, 2023

Choose a reason for hiding this comment

julienrf Jun 12, 2023

Choose a reason for hiding this comment

ckipp01 left a comment

Choose a reason for hiding this comment

julienrf commented Jun 12, 2023

Dedelweiss commented Jun 12, 2023

Dedelweiss commented Apr 11, 2023 •

edited

Loading

julienrf commented Apr 12, 2023 •

edited

Loading

Dedelweiss commented May 5, 2023 •

edited

Loading

Dedelweiss commented May 9, 2023 •

edited

Loading

Dedelweiss May 12, 2023 •

edited

Loading