Add URL rewrite for markdown docs#199
Merged
Merged
Conversation
Reject any render.linkMappings entry with an empty url, link, or text. An empty url is especially dangerous: strings.ReplaceAll(text, "", repl) inserts the replacement between every character, silently corrupting all rendered output. Failing fast at config load surfaces the misconfiguration clearly instead of producing garbage docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
RewriteLinks used strings.ReplaceAll, so when one mapped URL was a prefix
of another (".../old" vs ".../old-page"), correctness depended on the order
mappings happened to be declared in the config. Sort a copy of the mappings
by descending URL length before applying them, so the more specific URL
always wins regardless of config order. Update the prefix test to declare
the shorter URL first, proving order-independence.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Field docs and $type.Doc were rewritten, but group/version package doc
comments rendered raw via {{ $gv.Doc }}, so a mapped URL in a GV doc was
silently missed. Wrap $gv.Doc with markdownRewriteLinks in the bundled and
test markdown templates. Add a mapped URL to the test package comment to
exercise this end-to-end and regenerate the golden files (markdown rewrites
it, asciidoc leaves it raw as expected).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A mapped URL that already appears inside a Markdown link gets rewritten too, producing nested invalid Markdown. This is acceptable for the plain-URL godoc use case. Relabel the test to mark it a known limitation rather than desired behavior, and add a README caveat telling users not to map already-linked URLs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
RenderFieldDoc rewrites links before escaping the pipe character. Add a test exercising a field doc that both contains a mappable URL and a literal pipe, asserting the URL becomes a link and the pipe is still escaped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
linkMappings are inline in the config file, which is fine for small per-repo sets but can become unwieldy at scale. Document the trade-off rather than building external mapping-file support now. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Hi @shainaraskas, thanks for this! 🙏 Really clean addition, and the no-op-when-unconfigured design means existing consumers see zero behavior change. I pushed a few small follow-up commits on top of your branch:
Kept as separate commits for easy review, I would appreciate you taking a look and letting me know if anything doesn't sit right with you. Thanks again! |
Member
Author
|
@thbkrkr thanks for all of these updates. looked over all of the commits / reran the tests locally and it all lgtm. I have limited access to this repo, so please merge this when you think it's good to go. Would also love a preview of the release process: at what point would the maintainers consider cutting a new release? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Adds optional URL rewriting in the markdown renderer.
Consumers can declare
(url, link, text)triples underrender.linkMappingsin the config file. The markdown renderer rewrites occurrences of the url in field and type doc comments to[text](link)at render time. Lets us produce docs-builder cross-repo links from plain URLs in godoc instead of post-processing the rendered markdown in a downstream script.Replaces elastic/cloud-on-k8s#9454 - will follow up with mappings in cloud-on-k8s after this change is released
Changes
render.linkMappingsconfig fieldRewriteLinksmethod on the markdown rendererRenderFieldDoc; bundled markdown templates also call it on$type.Doc(which doesn't go throughRenderFieldDoc)I've left the asciidoc renderer alone:
Questions