Skip to content

Use the new JSDoc parser to handle @param tags #6867

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

Closed
wants to merge 2 commits into from

Conversation

RyanCavanaugh
Copy link
Member

Fixes #6810

We used to have two separate JSDoc Parameter Tag parsers -- one in services and one in parser. This PR removes the old tag parser in services and defers to the one in parser (which is more featureful and tested).

However, the parser doesn't extract doc comments (it leaves them as inter-node trivia), so we still need a function that extracts out the doc comments from a given JSDoc tag. This is now a general-purpose function getCommentFromJsDocTag that can do this for any kind of JSDoc tag (not just param tags).

parts.push(text.substr(start, rangeEnd - start));
}

const result: SymbolDisplayPart = {
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you just add a return type annotation and return the result directly?

@billti
Copy link
Member

billti commented Feb 3, 2016

Testing this with a JsDoc comment such as p1 below it breaks the function tooltip (it shows the tooltip as "@hotmail.com for more info @hotmail. @return {string} This is the result") :

/**
 * @param {string} p1 - A string param. 
 * Contact [email protected] for more info
 * @param {string?} p2 - An optional param 
 * @param {string} [p3] - Another optional param. me@hotmail.
 * @param {string} [p4="test"] - An optional param with a default value
 * @return {string} This is the result
 */
function f1(p1, p2, p3, p4){}

It seems to be caused by getCleanedJsDocComment, not specifically these changes, but in general looks like this code often treats @ anywhere as special char, not just after the initial optional whitespace and asterisk. Worth fixing up.

while (start < rangeEnd) {
const ch = text.charCodeAt(start);
if (ch === CharacterCodes.asterisk || isWhiteSpace(ch)) {
start++;
Copy link
Member

Choose a reason for hiding this comment

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

Adding the below to the if expression gets rid of the leading - in @param comments nicely for me 😄

|| (ch === CharacterCodes.minus && tag.kind === SyntaxKind.JSDocParameterTag)

@RyanCavanaugh
Copy link
Member Author

I've spent three straight days on this and it's still not handling all the use cases we're inventing during development. This isn't a P1 for the 1.8 Salsa release so I'm going to close this and we can re-address at a later date unless there are substantial objections.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants