Add external link redirect mechanism #2586
Conversation
scripts/js/lib/api/htmlToMd.ts
Outdated
| for (const redirect of externalRedirects) { | ||
| markdown = markdown.replaceAll(redirect.old, redirect.new); | ||
| } |
There was a problem hiding this comment.
This makes more sense in processHtml.ts, such as how we implement this:
Reasons:
a) It'd be good to have a unit test for this new functionality
b) It's less risky using Cheerio because we can be confident we're only changing the <a> and not anything unexpected
c) abstraction level: this function sphinxHtmlToMarkdown is meant to be very high level and having this replace breaks that abstraction.
There was a problem hiding this comment.
I agree with Eric that maybe this is better to implement in processHtml.ts 👍
There was a problem hiding this comment.
Ok done, the catch with <a> is that it'll only replace the href, not the link text. Is that the intended behaviour?
There was a problem hiding this comment.
It'd be good to replace the text as well if it's a match. You can use .text() to do that
documentation/scripts/js/lib/api/processHtml.ts
Lines 409 to 427 in 12a981b
Co-authored-by: Eric Arellano <[email protected]>
| }; | ||
| const doc = CheerioDoc.load( | ||
| `<a href="https://ibm.com/old">https://ibm.com/old</a> | ||
| <a href="https://basename.example-website.com/old#anchor?and=query"></a> |
There was a problem hiding this comment.
Can you please add a URL that is not redirected? Ideally it looks very similar to an old URL, such as https://ibm.com/old-but-not-redirected. We want to only change exact matches.
| const newText = $a.text().replaceAll(href, replacement); | ||
| $a.text(newText) |
There was a problem hiding this comment.
This should require an exact match, not partial
Co-authored-by: Eric Arellano <[email protected]>
Fixes #2579 by adding a mechanism to redirect links without needing to modify the artifact. This is less work for maintainers, and less risky as it means we don't need to edit the artifact as much.
A future PR could have the link checker push to
external-redirects.tswhen it detects a 301, but this should be fine for now.