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

Update: Add requireParamType option to valid-jsdoc (fixes #6753) #10220

Merged
merged 1 commit into from Jun 2, 2018
Merged

Update: Add requireParamType option to valid-jsdoc (fixes #6753) #10220

merged 1 commit into from Jun 2, 2018

Conversation

smokku
Copy link
Contributor

@smokku smokku commented Apr 15, 2018

This option allows to skip specyfing parameters type in JSDoc comments. It is usefull in cases, when there are other type annotations available already - i.e. Flow type.

Having types both in JSDoc and other annotation is redundant
and may cause inconsistency.

Example:

/*eslint valid-jsdoc: ["error", { "requireParamTypes": false, "requireReturnType": false }]*/

/**
 * Add two numbers.
 * @param num1 The first number.
 * @param num2 The second number.
 * @returns The sum of the two numbers.
 */
function add(num1: number, num2: number): number {
    return num1 + num2;
}

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What rule do you want to change?

valid-jsdoc

Does this change cause the rule to produce more or fewer warnings?

fewer

How will the change be implemented? (New option, new default behavior, etc.)?

new option

Please provide some example code that this change will affect:

The affected code examples are given in the rule documentation and commit message above.

What does the rule currently do for this code?

Generates Missing JSDoc parameter type for 'param'.

What will the rule do after it's changed?

Will not generate message.

What changes did you make? (Give an overview)

Implemented new option support, documentation and test.

Is there anything you'd like reviewers to focus on?

Nothing in particular. The change is trivial.

@jsf-clabot
Copy link

jsf-clabot commented Apr 15, 2018

CLA assistant check
All committers have signed the CLA.

@aladdin-add aladdin-add added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Apr 16, 2018
This option allows to skip specyfing parameters type in JSDoc
comments. It is usefull in cases, when there are other type
annotations available already - i.e. Flow type.
Having types both in JSDoc and other annotation is redundant
and may cause inconsistency.

Example:

```js
/*eslint valid-jsdoc: ["error", { "requireParamTypes": false, "requireReturnType": false }]*/

/**
 * Add two numbers.
 * @param num1 The first number.
 * @param num2 The second number.
 * @returns The sum of the two numbers.
 */
function add(num1: number, num2: number): number {
    return num1 + num2;
}
```
@platinumazure
Copy link
Member

Hi @smokku, thanks for the PR. On behalf of the ESLint team, I apologize for letting this sit so long.

Assuming JSDoc itself allows typeless parameter definitions (and it seems to), this seems like a reasonable enhancement and I'll support it. If you can show me a JSDoc spec fragment that says parameter types are optional, I might even champion this. Thanks!

@smokku
Copy link
Contributor Author

smokku commented May 28, 2018

http://usejsdoc.org/tags-param.html

The @param tag requires you to specify the name of the parameter you are documenting. You can also include the parameter's type, enclosed in curly brackets, and a description of the parameter.

@platinumazure platinumazure self-assigned this May 28, 2018
@platinumazure
Copy link
Member

Alright, I'll champion this PR. If we can get three more team members to 👍 it (and no 👎) then we can accept the proposal and, hopefully, merge soon after that.

Give me a few days, but feel free to ping me after that as a reminder. 😄 Thanks for your patience!

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Code/docs/tests LGTM, thanks! (Leaving this as a comment to avoid accidental merging before the proposal is accepted.)

@mysticatea
Copy link
Member

I didn't realize JSDoc type notations are optional. In that case, this PR looks reasonable.

@smokku
Copy link
Contributor Author

smokku commented May 30, 2018

Literally this says ... can ... include ... type ... and a description.

This is not and/or, so according to rules of English grammar you can skip both, not just one.

But since valid-jsdoc rule has requireParamDescription option, it already acts like these were optional separately. Tool I use ( https://github.com/documentationjs/documentation ) is fine with skipping one, other or both.

@platinumazure
Copy link
Member

Literally this says ... can ... include ... type ... and a description.

This is not and/or, so according to rules of English grammar you can skip both, not just one.

I don't think that's the only/most natural reading of the spec. It could also mean, can include a type and can include a description.

In any case, I think we can safely allow types, descriptions, both, or neither on parameters. Worst case, we're more flexible than the JSDoc spec.

platinumazure
platinumazure previously approved these changes May 31, 2018
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! (Now changing to "approve" since this is accepted. Oops, still not yet accepted. Sorry for the wasted cycles.)

Would still like another set of eyes before merging.

@platinumazure platinumazure dismissed their stale review May 31, 2018 21:08

Oops, issue is not accepted

@platinumazure
Copy link
Member

@eslint/eslint-team: Could we get more eyes on this proposal? The JSDoc spec is pretty clear here, and I think we should make the rule conform appropriately.

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 1, 2018
@kaicataldo
Copy link
Member

Huh, I didn't know this either. Change makes sense 👍. This is now accepted.

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@platinumazure platinumazure merged commit 917108d into eslint:master Jun 2, 2018
@smokku
Copy link
Contributor Author

smokku commented Jun 2, 2018

thx

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 1, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants