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

validate front matter #1054

Merged
merged 8 commits into from
Mar 25, 2024
Merged

validate front matter #1054

merged 8 commits into from
Mar 25, 2024

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Mar 14, 2024

  • error on unparsable front matter
  • validate data.sql shape, console warn if there is any issue
  • validate all the options known today

we could validate more stuff in the future

closes #1005
(although I could not reproduce the same crashes)

I had fun making sql tables named '123' and 'a"b': it worked. I would definitely not recommend, though.

@Fil Fil requested a review from mbostock March 14, 2024 16:01
@Fil Fil enabled auto-merge (squash) March 15, 2024 07:42
@Fil Fil force-pushed the fil/validate-sql-frontmatter branch from 9638cb9 to abbe0a4 Compare March 19, 2024 14:58
@Fil
Copy link
Contributor Author

Fil commented Mar 19, 2024

I've rebased this PR and added some sort of validation for all the expected fields. The story for toc is a bit complicated, since it gets merged with config.toc later on.

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

I pushed a commit which takes this in a slightly different direction:

  • Changes the MarkdownPage["data"] to the strongly-typed FrontMatter interface
  • Adds missing fields to the FrontMatter interface (e.g., sidebar, style)
  • Adds front matter normalization tests
  • Removes obsolete downstream coercion e.g. in mergeToc and mergeStyle
  • Removes warning logs/erroring in favor of deterministic normalization

The last part is in the spirit of HTML5, which is to say that the normalization is well-defined, regardless of whatever input types the user gives us, so we don’t really need to consider these as errors.

@mbostock mbostock disabled auto-merge March 24, 2024 03:39
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Approving but disabled auto merge so you have a chance to review. 🙏

@Fil
Copy link
Contributor Author

Fil commented Mar 24, 2024

We still need to catch any YAMLException, otherwise the server crashes when the front matter in malformed (which happens quite a lot while you're editing it, for example when you forget a colon). This is why I was returning a fake structure with an error message—it could be a structure like {error: "malformed yaml"} instead.

@Fil
Copy link
Contributor Author

Fil commented Mar 25, 2024

I'm afraid this looks a bit overengineered, but it now does what I wanted: show a clean error message when front matter is invalid; crash on build.

example error

@Fil Fil requested a review from mbostock March 25, 2024 10:52
@Fil Fil force-pushed the fil/validate-sql-frontmatter branch from 7aa61cc to 1b67f88 Compare March 25, 2024 11:03
@mbostock
Copy link
Member

I don’t think we should error. We should just ignore the front matter. (Markdown and HTML in general don’t error for bad syntax — it just gets ignored.)

@Fil
Copy link
Contributor Author

Fil commented Mar 25, 2024

OK, this simplifies things a lot.

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

I changed it so that if the front matter is invalid, it just gets treated as content so you can immediately see on the page that the front matter is broken. Also, this means we don’t need to log the buffer to the console, which I didn’t find useful anyway (since the buffer could be arbitrarily long — even the entire document — which meant that you can’t even see the error message in the console).

Screenshot 2024-03-25 at 9 39 02 AM Screenshot 2024-03-25 at 9 38 54 AM

@Fil
Copy link
Contributor Author

Fil commented Mar 25, 2024

thanks!

@Fil Fil merged commit 9d1e46f into main Mar 25, 2024
4 checks passed
@Fil Fil deleted the fil/validate-sql-frontmatter branch March 25, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate sql front matter
2 participants