Skip to content

fix getParameterSymbolFromJSDoc - #19355

Merged
Nathan Shively-Sanders (sandersn) merged 4 commits into
masterfrom
sandersn/fix-getParameterSymbolFromJSDoc
Oct 20, 2017
Merged

fix getParameterSymbolFromJSDoc#19355
Nathan Shively-Sanders (sandersn) merged 4 commits into
masterfrom
sandersn/fix-getParameterSymbolFromJSDoc

Conversation

@sandersn

Copy link
Copy Markdown
Member

getParameterSymbolFromJSDoc didn't correctly track back to the source of a @param tag in a lot of cases, because it didn't match the lookup code in getJSDocCommentsAndTags. Now both functions share the same utility functions.

Note that the utility functions return Node | undefined, and the top-down uses from getParameterSymbolFromJSDoc take advantage of the return value, while the bottom-up uses from getJSDocCommentsAndTags just use the truthiness. I can annotate it as such, but didn't in the initial pass.

Fixes #19268

@sandersn

Copy link
Copy Markdown
Member Author

Mohamed Hegazy (@mhegazy) we should put this in 2.6 because it fixes a new error for 2.6.

@mhegazy

Copy link
Copy Markdown
Contributor

Please port this to release-2.6

@sandersn
Nathan Shively-Sanders (sandersn) deleted the sandersn/fix-getParameterSymbolFromJSDoc branch October 20, 2017 02:35
@sandersn

Copy link
Copy Markdown
Member Author

It's now in release-2.6

@ajafff

Copy link
Copy Markdown
Contributor

Nathan Shively-Sanders (@sandersn) This changes how JSDoc comments apply to VariableStatements with multiple VariableDeclarations:

/** @deprecated */
var a, b;
a; // deprecated
b; // not deprecated, used to be deprecated in ts@<2.6.0

Is it intentional that JSDoc only applies to the first VariableDeclaration?

Comment thread src/compiler/utilities.ts
return getJSDocCommentsAndTags(node);
}

export function getSourceOfAssignment(node: Node): Node {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No need to export.

Please annotate potentially undefined return values (although as written this is potentially false).
I would prefer multiple statements with local variables instead of writing it all as one expression that has to be formatted on different lines anyway.

if (isExpressionStatement(node)) {
    const { expression } = node;
    if (isBinaryExpression(expression)) {
        const { operatorToken, right } = node;
        return operatorToken.kind === SyntaxKind.EqualsToken ? right : undefined;
    }
}

@ghost Deleted user (ghost) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I need to leave a comment indicating the requested changes.

@ghost

Copy link
Copy Markdown

May be obviated by #18832.

@sandersn

Copy link
Copy Markdown
Member Author

Ahmed Jaffery (@ajaff) It is intentional. Here are the reasons:

  1. After a big discussion in the Typescript room, there were more fussy people than messy ones (we do make a compiler, after all).
  2. We are also running a twitter poll and opinions are about evenly split there. So whatever we do, it will be surprising to about half of all people.
  3. As far as I can tell from closure discussions, the closure compiler behaves (behaved?) this way too, even issuing a warning.
  4. It's easier to implement this way. (This was the deciding factor given how mixed the other indicators were.)

@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 14, 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.

3 participants