-
Notifications
You must be signed in to change notification settings - Fork 126
Add check that page title is in sync with ToC, h1, and metadata #3669
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for contributing to Qiskit documentation! Before your PR can be merged, it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. Thanks! 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Thanks for your work on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is looking really good 🎉
Aside from a few small things, there's one change I think we should make before merging this:
At the moment, both collectInvalidImageErrors
and collectHeadingTitleMismatch
accept a markdown string, then parse that string into an abstract syntax tree. The parsing step is relatively slow, so I think we should pull that out of the functions so we only parse the tree once.
To do this, you'd move the parsing step to L92 of checkMarkdown.ts
, then change the two "collect..." functions to accept a tree: Root
rather than markdown: string
. Then tweak collectInvalidImageErrors
so it re-uses the same tree, rather than parsing again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, thanks again
function parseMarkdown(markdown: string): Root { | ||
return unified() | ||
.use(remarkParse) | ||
.use(remarkGfm) | ||
.use(remarkFrontmatter, ["yaml"]) | ||
.parse(markdown); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice helper function, what do you think about moving it to a new file (e.g. scripts/js/lib/markdownUtils.ts
) and re-using it in checkMarkdown.ts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment still seems relevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Great work. I really like the approach of combining checks for better performance.
@@ -0,0 +1,147 @@ | |||
// This code is a Qiskit project. | |||
// | |||
// (C) Copyright IBM 2024. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// (C) Copyright IBM 2024. | |
// (C) Copyright IBM 2025. |
type: "boolean", | ||
default: false, | ||
description: | ||
"Check files in the current and dev versions of the API docs have matching titles and metadata.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we check both titles and images, we shouldn't be overly specific
"Check files in the current and dev versions of the API docs have matching titles and metadata.", | |
"Check files in the current and dev versions of the API docs.", |
function parseMarkdown(markdown: string): Root { | ||
return unified() | ||
.use(remarkParse) | ||
.use(remarkGfm) | ||
.use(remarkFrontmatter, ["yaml"]) | ||
.parse(markdown); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment still seems relevant
#2489 Added a new function and test case for mismatched title and heading