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

feat(11378): Check @param names in JSDoc method documentation #47257

Merged
merged 1 commit into from
Feb 10, 2022

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #11378

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Dec 27, 2021
@sandersn sandersn self-assigned this Jan 14, 2022
@sandersn
Copy link
Member

A quick summary of the issue after re-reading it:

  1. "No matching parameter" error for @param tags should be issued in TS as an info.
  2. There should be a codefix to change the parameter name.
  3. The codefix should work for TS and JS.

@sandersn
Copy link
Member

@jakebailey I added you to the review because I can't remember if you've seen a codefix PR before. This is a nice simple one.

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 haven't looked at the codefix's code yet, but from looking at the tests, I wish that it were able to suggest a replacement parameter when a good match could be found. I need to think about whether this fix is worthwhile without it.
That involves estimating how often this codefix would be used on existing, incorrect param tags, and how those tags are incorrect. My initial thought is that most bad tags occur when they no longer match the function, and that tag should be changed, not the parameter list.

src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
@a-tarasyuk
Copy link
Contributor Author

@sandersn Thanks for the review.

My initial thought is that most bad tags occur when they no longer match the function, and that tag should be changed, not the parameter list.

Do you mean that we need to remove @param z instead of adding z to the signature?

/**     
  * @param x     first summand
  * @param y     second summand
  * @param z     third summand
  */
function sum (a: number, b: number) : number {
    return a + b;
}

... should be issued in TS as an info

Do you mean DiagnosticCategory.Suggestion? Or do we need to add new option to DiagnosticCategory?

src/compiler/checker.ts Outdated Show resolved Hide resolved
@jakebailey
Copy link
Member

Maybe I'm misreading the tests, but the suggestion added seems backward to me. Isn't it more likely that the code itself is "correct" and it's the JSDoc that should be updated, not the other way around?

@a-tarasyuk
Copy link
Contributor Author

@jakebailey I added a question to clarify this logic. If need to change a JSDoc instead of the signature, I will change the QF.

@sandersn
Copy link
Member

I agree with @jakebailey. I think the codefix should assume that the parameter list is correct and try to make the @param tags match it.

@a-tarasyuk a-tarasyuk force-pushed the feat/11378 branch 2 times, most recently from 9bf5123 to 0f5cc8f Compare January 27, 2022 22:08
@sandersn
Copy link
Member

@a-tarasyuk, @gabritto pointed out that I missed your question in
#47257 (comment)

  1. Yes, I think the tag should be deleted instead of the parameter.
  2. I meant Suggestion but used the emacs term Info by mistake. I didn't mean to suggest creating yet another level of errors. =]

@a-tarasyuk
Copy link
Contributor Author

@sanders Thanks. Special thanks to @gabritto for the friendly reminder ;).

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.

Looks good, just a few minor style things.

src/services/codefixes/fixUnmatchedParameter.ts Outdated Show resolved Hide resolved
src/compiler/diagnosticMessages.json Outdated Show resolved Hide resolved
src/services/codefixes/fixUnmatchedParameter.ts Outdated Show resolved Hide resolved
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

This all looks good to me, though I don't have much experience with these code fix PRs. Is there anything special needed if "fix all" support is desired, or is that already magically implemented (or, not needed anyhow?).

@sandersn Can take a peek to see if he's good with the changes.

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.

Looks good, just one more wrinkle I noticed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Check @param names in JSDoc method documentation
5 participants