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

Add helper for Markdown inline code escaping #18867

Closed
wants to merge 5 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Jul 9, 2023

The helper adds escaping to a string so that it can be embedded in a Markdown inline code segment delimited by backticks.

@fmeum fmeum force-pushed the stardoc-escape branch from 7316ec4 to c28f179 Compare July 9, 2023 09:27
@fmeum fmeum marked this pull request as ready for review July 9, 2023 09:34
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Jul 9, 2023
@sgowroji sgowroji added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jul 10, 2023
@tetromino tetromino self-assigned this Jul 11, 2023
Copy link
Contributor

@tetromino tetromino left a comment

Choose a reason for hiding this comment

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

I agree with the PR in principle, but I would ask for some changes.

  1. Terminology nit: GH-flavored-markdown's official name for this is "code span", so let's rename the method to markdownCodeSpan
  2. Please include the outermost backticks in the function's output. The only reason to not include them would be to allow concatenating multiple strings within a single span, e.g. to have`${util.markdownCodeSpan(foo)}${util.markdownCodeSpan(bar)}` in a template - but that is impossible to do safely (consider what happens when ${util.markdownCodeSpan(foo)} outputs a string with a terminal sequence of `-s). Omitting the outermost onion layer of backticks is therefore a footgun for careless template writers.
  3. We'd want a unit test in src/test/java/com/google/devtools/build/skydoc/SkydocTest.java on second thought, let's create a src/test/java/com/google/devtools/build/skydoc/MarkdownUtilTest.java

@tetromino
Copy link
Contributor

Note that I'm in the process of moving the renderer code from Bazel into the Stardoc repo; if you don't have time to make those changes, I can do them myself before the move (to avoid merge complications).

fmeum added 2 commits July 12, 2023 00:21
The helper adds escaping to a string so that it can be embedded in a
Markdown inline code segment delimited by backticks.
@fmeum fmeum requested a review from tetromino July 12, 2023 07:29
@fmeum
Copy link
Collaborator Author

fmeum commented Jul 12, 2023

Note that I'm in the process of moving the renderer code from Bazel into the Stardoc repo; if you don't have time to make those changes, I can do them myself before the move (to avoid merge complications).

I made the changes you requested above, but I'm not sure what I would/would not have to do to support the move.

Add missing license comment; make method static; make javadoc easier to read;
run through formatter/linter.
Copy link
Contributor

@tetromino tetromino left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good. I took the liberty of adding the missing srcs filegroup and fixing some stylistic nits / linter suggestions.

@fmeum
Copy link
Collaborator Author

fmeum commented Jul 12, 2023

@tetromino Thanks! Looks like there is a visibility issue with that though.

@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jul 12, 2023
@fmeum fmeum deleted the stardoc-escape branch July 12, 2023 17:11
tetromino pushed a commit to bazelbuild/stardoc that referenced this pull request Jul 18, 2023
Reverts #133 so that HTML escaping is not applied to Markdown. Instead, Markdown content such as docstrings can use HTML formatting and escape angle brackets with backslashes, HTML entities or inline code segments. Default values are embedded in inline code segments instead of `<code>` tags, which does not require escaping.

As a result, docstrings behave just like regular Markdown contexts while default values are rendered without smart quotes and can contain both `<` and `` ` `` without causing escaping issues.

Also includes tests based on #138.

Fixes #137
Closes #138
Fixes #142
Closes #143

Requires bazelbuild/bazel#18867

Co-authored-by: Adam Azarchs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants