-
Notifications
You must be signed in to change notification settings - Fork 157
fix: Preserve @example titles
#476
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
base: main
Are you sure you want to change the base?
Changes from all commits
ef8dac1
0e29d6a
80f3f4a
66f2b8a
7bbc436
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "changes": [ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The bug being fixed, restatedParse a comment containing Input: /**
* @example Adding two numbers
* ```ts
* add(1, 2);
* ```
*/Emitted output: /**
* @example
*
* Adding two numbers
* ```ts
* add(1, 2);
* ```
*
*/That is an emitter layout bug, not a parser bug. The text was never lost, it just came back in the wrong place. The fix for it is much smaller than the fix for "expose the title on the AST", and the two should not be entangled.
One caveat if you do this. It needs a guard for the untitled case. Naively adding Compatibility: keep the title in
|
||
| { | ||
| "packageName": "@microsoft/tsdoc", | ||
| "comment": "Fix round-trip emission of `@example` block titles so the title text is preserved on the tag line, and expose the text on a block's tag line via new `DocBlock.tagLineContent` and `DocBlock.bodyContent` accessors", | ||
| "type": "minor" | ||
| } | ||
| ], | ||
| "packageName": "@microsoft/tsdoc" | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Four exhibits, and what I'm thinking each one should do.
Exhibit A: a title above a code sample.
Title is
Adding two numbers. A documentation tool should typeset it as the heading for this example, in place of the genericExample 2numbering. This is a clarification, not a change: tsdoc.org already says text on the tag line is the title. Nothing parses differently, we are just exposing what is already there.Exhibit B: the whole example is on the tag line.
Read literally, the spec says
`"AMD64"`is a title for an example with no body. That is clearly not what the author meant, and this idiom appears about a dozen times in rushstack alone (rush-lib/src/logic/Telemetry.ts,node-core-library/src/JsonSchema.ts,lockfile-explorer-web). Our call: parse it identically to Exhibit A, so no author has to change anything, and let the renderer notice the body is empty and render the line inline rather than as a heading. This is the one place we are adding to the spec, and we are adding a rendering convention rather than a parsing rule.Exhibit C: markup in the title.
The markup is markup, not literal text.
{@link add}is a realDocLinkTagthat API Extractor resolves and validates, and a code span in the same position is aDocCodeSpan. That is how it parses today and it must keep working. The spec is silent on the point, so we are filling a gap rather than changing anything.This is the reason the title has to be a list of nodes rather than a string. A
stringtitle can only hold the literal charactersUsing {@link add} on negative numbers, which demotes the link to text, drops it out of API Extractor's reference validation, and leaves every renderer to either print the braces or re-parse the string itself. Same argument for a backslash escape on the title line.Exhibit D: a modifier tag on the tag line.
@internalstays a modifier tag, exactly as it does today. The title isAdding two numbersand stops there, because the block itself ends at the second tag. This is not a new rule, it is a guarantee that the title feature does not quietly repeal an existing one. API Extractor derives every release tag frommodifierTagSet, so getting this wrong changes whether an API is trimmed from a release build.Deferred to a future revision. It would be nice to have a restricted grammar for titles, with a diagnostic when the title line contains something that cannot be a title. It would be nice to normalize or deprecate Exhibit B rather than living with a rendering convention. It would be nice to settle
@throws, whose own page describes a next-line convention while the tag kinds page claims it behaves like@example. And it would be nice to have a general mechanism, such as a flag onTSDocTagDefinition, so that first-line-is-a-title is declared per tag rather than hardcoded inNodeParser. None of these should hold up this PR. All of them need a deprecation cycle and a spec pass that this change cannot carry.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Joshua Smithrud (@Josmithr) what do you think? Do you agree with these calls? Will they cause any migration trouble for your codebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, all of those make sense to me. I'd noted the markup limitation in the PR description and suspected it might be something we wanted to tackle here. The other cases are not ones I considered, and your reasoning makes sense to me. I'll go ahead and make these updates (and clarify details in the documentation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.