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

fix getParameterSymbolFromJSDoc #19355

Merged
merged 4 commits into from
Oct 20, 2017

Conversation

sandersn
Copy link
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

They are now used both in getJSDocCommentsAndTagsWorker and in
geParameterSymbolFromJSDoc.
@sandersn
Copy link
Member Author

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

@mhegazy
Copy link
Contributor

mhegazy commented Oct 19, 2017

Please port this to release-2.6

@sandersn sandersn merged commit 1ea1254 into master Oct 20, 2017
@sandersn sandersn deleted the sandersn/fix-getParameterSymbolFromJSDoc branch October 20, 2017 02:35
@sandersn
Copy link
Member Author

It's now in release-2.6

@ajafff
Copy link
Contributor

ajafff commented Oct 20, 2017

@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?

@@ -1538,6 +1538,34 @@ namespace ts {
return getJSDocCommentsAndTags(node);
}

export function getSourceOfAssignment(node: Node): Node {
Copy link

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;
    }
}

Copy link

@ghost ghost left a comment

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

ghost commented Oct 20, 2017

May be obviated by #18832.

@sandersn
Copy link
Member Author

@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 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