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

Extend the images' checker to check API docs #2426

Merged
merged 3 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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 .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ jobs:
- name: File metadata
run: npm run check:metadata
- name: Check images
run: npm run check:images
run: npm run check:images -- --apis
Copy link
Collaborator

Choose a reason for hiding this comment

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

Insted, this should go into https://github.com/Qiskit/documentation/blob/main/.github/workflows/api-checks.yml. We want the main job to be as fast as possible for non-API changes. We're okay with API changes being slightly slower (repeating the check of non-API)

- name: Spellcheck
run: npm run check:spelling
- name: Check Qiskit bot config
Expand Down
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,10 @@ You can check it locally by running:
# Only check images
npm run check:images

# By default, only the non-API docs are checked. You can add the
# below argument to also check API docs.
npm run check:images -- --apis

# Or, run all the checks
npm run check
```
Expand Down
57 changes: 54 additions & 3 deletions scripts/js/commands/checkImages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,43 @@
// that they have been altered from the originals.

import { globby } from "globby";
import yargs from "yargs/yargs";
import { hideBin } from "yargs/helpers";
import { collectInvalidImageErrors } from "../lib/markdownImages.js";
import { readMarkdown } from "../lib/markdownReader.js";

// Known APIs containing images without alt text that will be skipped
// by the checker.
const APIS_TO_IGNORE__SHOULD_FIX: string[] = [
"qiskit",
"qiskit-ibm-runtime",
"qiskit-addon-cutting",
"qiskit-addon-mpf",
"qiskit-addon-obp",
"qiskit-addon-utils",
];

interface Arguments {
[x: string]: unknown;
apis: boolean;
}

const readArgs = (): Arguments => {
return yargs(hideBin(process.argv))
.version(false)
.option("apis", {
type: "boolean",
default: false,
description:
"Check the images in the current and dev versions of the API docs.",
arnaucasau marked this conversation as resolved.
Show resolved Hide resolved
})
.parseSync();
};

async function main() {
const files = await globby(["docs/**/*.{ipynb,mdx}", "!docs/api/**/*.mdx"]);
const args = readArgs();

const files = await determineTocFiles(args);
const fileErrors: string[] = [];

for (const file of files) {
Expand All @@ -24,19 +56,38 @@ async function main() {

if (imageErrors.size) {
fileErrors.push(
`Error in file '${file}':\n\t- ${[...imageErrors].join("\n\t- ")}`,
`Error in file '${file}':\n\t- ${[...imageErrors].join("\n\t- ")}\n`,
);
}
}

if (fileErrors.length) {
fileErrors.forEach((error) => console.log(error));
console.error(
"\n💔 Some images have problems. See https://github.com/Qiskit/documentation#images for instructions.\n",
"💔 Some images have problems. See https://github.com/Qiskit/documentation#images for instructions.\n",
);
process.exit(1);
}
console.log("✅ All images are valid.\n");
}

async function determineTocFiles(args: Arguments): Promise<string[]> {
// We always skip historical versions to simplify the code and to have a faster script.
// Even though it is possible for someone to add a new image without alt text to a
// historical version that wasn't in the original release, that's very unlikely.
// If it happens, it would probably be a backport from latest or dev, and the linter in
// the API repo should catch it.
//
// If an image is missed by the API repo's linter, it will still have an alt text defined,
// although it won't be very useful.
Comment on lines +81 to +82
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be helpful to add

That's because Sphinx auto-generates alt text.

const globs = [
"docs/**/*.{ipynb,mdx}",
"!docs/api/qiskit/release-notes/*.mdx",
args.apis ? "!docs/api/*/([0-9]*)/*.mdx" : "!docs/api/**/*.mdx",
arnaucasau marked this conversation as resolved.
Show resolved Hide resolved
...APIS_TO_IGNORE__SHOULD_FIX.map((api) => `!docs/api/${api}/**/*.mdx`),
];

return await globby(globs);
}

main().then(() => process.exit());
3 changes: 3 additions & 0 deletions scripts/js/lib/markdownImages.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,16 @@ test("Test the finding of invalid images", async () => {
<img src="/images/HTMLexample2.jpg" alt="Example" width="200"/>

![And now, our last link](https://ibm.com)

![../\_images/invalid\_img3.png](/images/invalid_img3.png)
`;
const images = await collectInvalidImageErrors(markdown);
const correct_images = new Set([
"The image '/images/HTMLexample1.jpg' uses an HTML <img> tag instead of markdown syntax.",
"The image '/images/HTMLexample2.jpg' uses an HTML <img> tag instead of markdown syntax.",
"The image '/images/invalid_img1.png' does not have alt text.",
"The image '/images/invalid_img2.png' does not have alt text.",
"The image '/images/invalid_img3.png' does not have alt text.",
]);

expect(images).toEqual(correct_images);
Expand Down
6 changes: 5 additions & 1 deletion scripts/js/lib/markdownImages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { visit } from "unist-util-visit";
import remarkParse from "remark-parse";
import remarkGfm from "remark-gfm";
import remarkStringify from "remark-stringify";
import { last, split } from "lodash-es";

export async function collectInvalidImageErrors(
markdown: string,
Expand All @@ -28,7 +29,10 @@ export async function collectInvalidImageErrors(
.use(remarkGfm)
.use(() => (tree: Root) => {
visit(tree, "image", (node) => {
if (!node.alt) {
// Sphinx uses the image path as alt text if it wasn't defined using the
// :alt: option.
const imageName = last(split(node.url, "/"));
if (!node.alt || node.alt.endsWith(imageName!)) {
Comment on lines +32 to +35
Copy link
Member

Choose a reason for hiding this comment

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

Good call

imagesErrors.add(`The image '${node.url}' does not have alt text.`);
}
});
Expand Down
Loading