Skip to content

Check JSDoc @param tag names #18777

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

Merged
2 commits merged into from
Sep 28, 2017
Merged

Check JSDoc @param tag names #18777

2 commits merged into from
Sep 28, 2017

Conversation

ghost
Copy link

@ghost ghost commented Sep 26, 2017

Related: #15852, but that's a broader issue so this won't fix it completely.

@ghost ghost requested a review from sandersn September 26, 2017 21:45
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I don't think there should be an error in Typescript, or in cases where there is no typeExpression. Well, they should be warnings, if we had such things already.

Either way, I'd like to see tests for these cases.

function checkJSDocParameterTag(node: JSDocParameterTag) {
checkSourceElement(node.typeExpression);
if (!getParameterSymbolFromJSDoc(node)) {
error(node.name,
Copy link
Member

Choose a reason for hiding this comment

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

This should be a lint-style warning if there is no typeExpression, or if the typeExpression is ignored, the way it is in Typescript.
Until we have a warning infrastructure, I would rather not issue the error in those two cases.

@@ -19789,6 +19789,15 @@ namespace ts {
}
}

function checkJSDocParameterTag(node: JSDocParameterTag) {
checkSourceElement(node.typeExpression);
if (!getParameterSymbolFromJSDoc(node)) {
Copy link
Member

Choose a reason for hiding this comment

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

getParameterSymbolFromJSDoc has a linear search of function.parameters, and we call this for every @param node. So it's quadratic where it could be linear if we wrote sufficiently clever code.

Might not be an issue, but I wanted to make sure you'd thought about it. (Most functions have few parameters and most @params are correct, so pathological cases should be rare.)

@ghost
Copy link
Author

ghost commented Sep 27, 2017

@sandersn Based on our discussion I think all of these points can be dealt with later? I could start working on performance tomorrow if this got in today.
To summarize:

  • Won't apply in TypeScript because we never visit the node.
  • Performance for matching @params with actual params is already terrible. We might fix this by requiring correct ordering or else issuing an error.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Yep, sounds good to me.

@ghost ghost force-pushed the check-param branch from 410dbfa to f0325ab Compare September 28, 2017 20:32
@ghost ghost merged commit 7959bd0 into master Sep 28, 2017
@ghost ghost deleted the check-param branch September 28, 2017 20:44
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
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.

1 participant