Skip to content

improvement(api-markdown-documenter): Don't needlessly escape > characters in Markdown #24813

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

Conversation

Josmithr
Copy link
Contributor

Escaping of > is not strictly required in Markdown. > only has special meaning if it is closing an unescaped <. So, as long as we continue to escape the latter, we don't need to escape the former.

The primary goal of this PR is to align our Markdown rendering with the behavior of mdast-util-to-markdown, which we plan to use to replace our custom Markdown rendering.

@Josmithr Josmithr requested review from alexvy86, jumyhre, RishhiB, a team and Copilot June 10, 2025 19:17
@github-actions github-actions bot added the base: main PRs targeted against main branch label Jun 10, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the Markdown rendering logic and corresponding snapshots to stop escaping the > character, aligning behavior with mdast-util-to-markdown.

  • Remove > from the set of escaped characters in escapeTextForMarkdown
  • Update all snapshot tests to reflect unescaped > characters in breadcrumb links and types

Reviewed Changes

Copilot reviewed 110 out of 110 changed files in this pull request and generated no comments.

File Description
tools/api-markdown-documenter/src/renderers/markdown-renderer/default-renderers/RenderPlainText.ts Drop > from escape regex to avoid needless escaping
All snapshot files under tools/api-markdown-documenter/src/test/snapshots/markdown/... Updated expected output to remove backslashes before > in links and code blocks
Comments suppressed due to low confidence (2)

tools/api-markdown-documenter/src/renderers/markdown-renderer/default-renderers/RenderPlainText.ts:120

  • [nitpick] The comment still mentions escaping any special characters but the regex no longer escapes >. Consider updating this comment to note that > is intentionally left unescaped.
.replace(/[#&*<[]\]_`|~]/g, (x) => `\\${x}`) // then escape any special characters

tools/api-markdown-documenter/src/renderers/markdown-renderer/default-renderers/RenderPlainText.ts:118

  • There aren’t any direct unit tests confirming that > is no longer escaped (and that < still is). Adding tests for those cases will help prevent future regressions.
export function escapeTextForMarkdown(text: string): string {

@Josmithr Josmithr enabled auto-merge (squash) June 10, 2025 22:15
Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  223740 links
    1709 destination URLs
    1941 URLs ignored
       0 warnings
       0 errors


@Josmithr Josmithr merged commit f8fb70b into microsoft:main Jun 10, 2025
31 checks passed
@Josmithr Josmithr deleted the api-markdown-documenter/dont-escape-closing-caret branch June 10, 2025 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants