Add script to check links #173

frankharkins · 2023-10-16T14:17:13Z

This PR adds a link-checking script to verify internal links work. It should be easy to extend this to check external links, and anchors within internal links. All comments welcome.

Details

Uses markdown-link-extractor, which is used by markdown-link-check (one of the proposed tools).

First part of #4

…H/link-checker

Eric-Arellano

Thank you! This looks great 🙌

We'll also want this hooked up to package.json as check:links, and to add it to GitHub Actions so we error on invalid links. Include instructions in the README too.

Eric-Arellano · 2023-10-16T18:31:07Z

package.json

@@ -42,5 +42,8 @@
    "unified": "^10.0.0",
    "unist-util-visit": "^4.0.0",
    "zx": "^7.2.3"
+  },
+  "dependencies": {
+    "markdown-link-extractor": "^3.1.0"


Better as a dev dependency.

scripts/commands/checkLinks.ts

Eric-Arellano · 2023-10-16T20:52:27Z

scripts/commands/checkLinks.ts

+// that they have been altered from the originals.
+
+import { globby } from "globby";
+const { existsSync, readFileSync } = require('fs');


Better is to use the async versions:

documentation/scripts/lib/fs.ts

Line 13 in a1a72fe

import { stat } from 'fs/promises';

Async is nice for two reasons:

Makes more explicit what is doing a side-effecty and often slower thing like reading the file system

Allows us to run multiple futures at once using Promise.all(). It will allow us to check every link in concurrency, which is going to be a lot faster.

Allows us to run multiple futures at once using Promise.all(). It will allow us to check every link in concurrency, which is going to be a lot faster.

That's not going to be relevant if you take my below recommendation to pre-read all the valid files, since they will be synchronous operations. That's fine though - it will be really fast.

It's still good to use async here for consistent style.

scripts/commands/checkLinks.ts

Eric-Arellano · 2023-10-16T21:01:48Z

scripts/commands/checkLinks.ts

+    console.log(`❌ ${this.origin}: Could not find link '${this.value}'`)
+    return false


Instead of logging directly in this function, it might be a cleaner user interface to collect all the failing links in the main function or checkLinksInFile, and then bundle them together as a single console error, where you list all the failures.

This is a lot of repeated warnings right now, which can be noisy.

See an example:

documentation/scripts/commands/checkMetadata.ts

Lines 55 to 65 in a1a72fe

const notebookErrors = [];

const notebookFiles = await globby("docs/**/*.ipynb");

for (const file of notebookFiles) {

if (IGNORED_FILES.has(file)) {

continue;

}

const metadata = await readMetadata(file);

if (!isValidMetadata(metadata)) {

notebookErrors.push(file);

}

}

You'd have this function return something like structured data or a string representing what the failure is. Return undefined if it's okay. Return type is string?.

Eric-Arellano · 2023-10-16T21:09:23Z

scripts/commands/checkLinks.ts

+  let allGood = true
+  for (let link of links) {
+    allGood = link.check() && allGood
+  }


return links.all(link => link.check())

If you take the above suggestion to have check() return the error or undefineed, rather than directly logging it, then this would be return links.map(link => link.check()).filter(Boolean), where .filter(Boolean) gets rid of the undefined.

With links.every, the script will exit on the first broken link it finds, whereas the current loop prints all broken links. Is exiting early ok?

Ah, okay, never mind. We definitely do want to collect every single link. The .map().filter() suggestion would do that properly though, if you end up returning the error rather than logging directly.

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

* Reduces use of `existSync` by ~98%

Eric-Arellano

I like how this is progressing! The Link class is quite nice.

Eric-Arellano · 2023-10-18T12:47:22Z

scripts/commands/checkLinks.ts

+}
+
+function checkLinksInFile(filePath: string, filePaths: string[]): boolean {
+  const source = readFileSync(filePath, {encoding: 'utf8'})


We'll still want this to be an async read, which will require marking async function checkLinksInFile. This line will become const source = await readFile(...)

Then, lines 130-133 will become:

const results = await Promise.all(filePaths.map(fp => checkLinksInFile(fp, filePaths)) if (results.some(x => !x)) { }

That will parallelize things.

Eric-Arellano · 2023-10-18T12:50:11Z

scripts/commands/checkLinks.ts

+const DOCS_ROOT = "./docs"
+const CONTENT_FILE_EXTENSIONS = [".md", ".mdx", ".ipynb"]
+
+class Link {


It would be super valuable to have some unit tests for Link.resolve() and Link.check(). They don't have any side effects anymore and they're now pure functions, so it should be easy to test.

To do so, you'd add a file scripts/commands/linkChecker.ts or something like that. Copy this code over. Then import it from your commands file. And add a test file similar to dedupeIds.test.ts.

npm test to ensure the test works.

Eric-Arellano · 2023-10-18T12:51:10Z

scripts/commands/checkLinks.ts

+        return true
+      }
+    }
+    // Check disk for files not in cache (images etc.)


Ah, I was thinking the images should be in the cache too. The only files that we expect you to use locally are images from public/images and other docs/ files. Otherwise it's an error.

Eric-Arellano

We're going to merge this now to help out the content team. Frank will make a follow up PR to make some of the suggested refactors

This PR adds a link-checking script to verify internal links work. It should be easy to extend this to check external links, and anchors within internal links. All comments welcome. ### Details Uses `markdown-link-extractor`, which is used by `markdown-link-check` (one of the proposed tools). *** First part of #4

frankharkins added 2 commits October 16, 2023 15:06

First draft of link checker

6bcb1b2

Merge branch 'main' of https://github.com/Qiskit/documentation into F…

2fa5910

…H/link-checker

frankharkins requested a review from Eric-Arellano October 16, 2023 14:17

github-actions bot deployed to Preview October 16, 2023 14:17 View deployment

Fix lint

de8994f

github-actions bot deployed to Preview October 16, 2023 14:58 View deployment

Fix comments

870ad81

github-actions bot deployed to Preview October 16, 2023 16:27 View deployment

Eric-Arellano reviewed Oct 16, 2023

View reviewed changes

frankharkins and others added 2 commits October 18, 2023 13:32

Incorporate feedback

40572ef

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

Use cache of known content files to avoid disk read

87c28cc

* Reduces use of `existSync` by ~98%

github-actions bot deployed to Preview October 18, 2023 12:33 View deployment

Eric-Arellano reviewed Oct 18, 2023

View reviewed changes

Eric-Arellano mentioned this pull request Oct 18, 2023

Set up link checker #4

Closed

Eric-Arellano approved these changes Oct 19, 2023

View reviewed changes

Merge branch 'main' into FH/link-checker

107559c

github-actions bot deployed to Preview October 19, 2023 13:59 View deployment

Eric-Arellano merged commit 9bb5431 into main Oct 19, 2023
3 checks passed

Eric-Arellano deleted the FH/link-checker branch October 19, 2023 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add script to check links #173

Add script to check links #173

frankharkins commented Oct 16, 2023 •

edited by Eric-Arellano

Loading

Eric-Arellano left a comment

Eric-Arellano Oct 16, 2023

Eric-Arellano Oct 16, 2023

Eric-Arellano Oct 16, 2023

Eric-Arellano Oct 16, 2023

Eric-Arellano Oct 16, 2023

frankharkins Oct 17, 2023

Eric-Arellano Oct 17, 2023

Eric-Arellano left a comment

Eric-Arellano Oct 18, 2023

Eric-Arellano Oct 18, 2023

Eric-Arellano Oct 18, 2023

Eric-Arellano left a comment

		console.log(`❌ ${this.origin}: Could not find link '${this.value}'`)
		return false

	const notebookErrors = [];
	const notebookFiles = await globby("docs/*/.ipynb");
	for (const file of notebookFiles) {
	if (IGNORED_FILES.has(file)) {
	continue;
	}
	const metadata = await readMetadata(file);
	if (!isValidMetadata(metadata)) {
	notebookErrors.push(file);
	}
	}

Add script to check links #173

Add script to check links #173

Conversation

frankharkins commented Oct 16, 2023 • edited by Eric-Arellano Loading

Details

Eric-Arellano left a 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

Eric-Arellano left a 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

Eric-Arellano left a comment

Choose a reason for hiding this comment

frankharkins commented Oct 16, 2023 •

edited by Eric-Arellano

Loading