Investigate switching to eslint-plugin-jsdoc
Categories
(Developer Infrastructure :: Lint and Formatting, enhancement, P2)
Tracking
(firefox108 wontfix, firefox109 fixed)
People
(Reporter: standard8, Assigned: trickypr)
References
(Blocks 2 open bugs)
Details
Attachments
(20 files, 3 obsolete files)
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
Bug 1510561 - Part 2: Apply `plugin:mozilla/valid-jsdoc` to `browser/components/places`. r?Standard8
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
Bug 1510561 - Part 4: Apply `plugin:mozilla/valid-jsdoc` to `browser/components/search`. r?Standard8
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
Bug 1510561 - Part 6: Apply `plugin:mozilla/valid-jsdoc` to `browser/components/urlbar`. r?Standard8
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review |
|
Reporter | |
Comment 1•5 years ago
|
||
|
||
Updated•2 years ago
|
I would be interested in working on this if no one else is.
|
|
Reporter | |
Comment 3•2 years ago
|
||
(In reply to trickypr from comment #2)
I would be interested in working on this if no one else is.
Hi, thank you for the offer. This needs a little bit of work ahead before we are ready to implement - we also want to bring consistency for our rules across the tree, so we need to create a couple of configurations for the new rules which are then used everywhere.
If you are willing I think the starting point for this would be to go through the list of existing valid-jsdoc and require-jsdoc rules that are enabled across mozilla-central, and come up with a list of what the common (or mostly common) parts are, and then to map those to a list of rules in eslint-plugin-jsdoc.
It would probably be worth keeping the "valid" and "require" requirements as separate lists for now. It might be worth keeping a list of non-common rules that are used in various places, in case there's something else to enable.
We'll need to some level of agreement on the basic lists, but if they're fairly well matching what we have already, that hopefully shouldn't take too long.
Once we've got the lists, then we can work on the actual implementation.
If this is a bit more than you were expecting, I quite understand.
For the most part, those who are using jsdoc rules have a consistent rule set
Valid JSDoc
Status: Enabled, error
preferences:
return->returnsBoolean->booleanNumber->numberString->stringbool->boolean
requireParamDescription: false
requireReturn: false
requireReturnDescription: false
Varients
browser/components/newtab is disabled and has no rewriting preferences.
browser/components/pagedata has requireParamDescription set to true
debugger/client is disabled and has no rules
security and taskcluster/docker/periodic-updates do not have rewriting preferences
Require JSDoc
Status: enabled, error
FunctionDeclaration: false
MethodDefinition: false
ClassDeclaration: true
ArrowFunctionExpression: false
FunctionExpression: false
Varients
browser/components/newtab is disabled and has no rules
browser/components/pagedata has the following changes:
FunctionDeclaration:trueMethodDefinition:true
There are less users of require-jsdoc than there are valid-jsdoc
eslint-plugin-jsdoc config
{
"rules": {},
"settings": {
"jsdoc": {
"tagNamePreference": {
"return": "returns"
},
"preferredTypes": {
"Boolean": "boolean",
"Number": "number",
"String": string,
"bool": "boolean"
}
}
}
}
I don't think the new plugin has as valid controls as the old system, so I can't replicate require-jsdoc.
eslint-plugin-jsdoc seems to be stricter than the old system. There
are a few places where it caught errors that the old linter didn't and
that seemed valid, so I fixed them.
|
||
Updated•2 years ago
|
|
|
Reporter | |
Comment 6•2 years ago
|
||
Thank you for starting this off, it is great to see. I'd like to structure this a little differently so that we can make it easier to roll-out, and I think we'll want some more rules enabled by default.
Here's what I suggest:
- I'd like to separate out adding the node module for eslint-plugin-jsdoc into a separate bug - I would like to generate the patch for that bit, as it'll require some additional work on our side so that automation correctly picks up the module, and there's some extra pieces/updates I want to include. I should be able to do that quite quickly.
- I'd like to start putting these rules into two configurations, which we can then use across the repository, e.g. "valid-jsdoc" and "require-jsdoc". To do this:
- Create new configs alongside our existing ones
- Reference those in the index
- These can then be referenced in
.eslintrc.jsfiles via:
extends: [
"plugin:mozilla/valid-jsdoc",
"plugin:mozilla/require-jsdoc",
],
I'm suggesting two configurations for now, because I think that rolling out valid-jsdoc across the whole tree (though not as part of this bug!) is going to be much easier than require-jsdoc.
- In the valid-jsdoc configuration, I think we need to include more rules, these are probably all the ones we should have there:
"jsdoc/check-access": "error",
"jsdoc/check-param-names": "error",
"jsdoc/check-property-names": "error"
"jsdoc/check-tag-names": "error",
"jsdoc/check-types": "error",
"jsdoc/require-param-type": "error",
"jsdoc/require-returns-type": "error",
"jsdoc/valid-types": "error",
Although require-param-type and require-returns-type could be seen as part of the require section, I think that having the type specified is part of having a valid jsdoc, so if you're specifying one of those tags, you should also be specifying the type.
The other rules can go in the require-jsdoc configuration.
Does that sound reasonable?
|
|
||
Updated•2 years ago
|
That seems like a cleaner way of implementing it.
The only problem I would raise is that the configuration above is strict (82 errors, 42 fixable on places). From what I can see, most of the errors are legitimate problems. Once Bug 1792465 has been fixed, I will create a new patch.
|
|
Reporter | |
Comment 8•2 years ago
|
||
I think it is fine that we go a little bit stricter - I'd rather go stricter as we roll it out jsdoc to more locations and have consistency, than to try and bring that in later.
|
|
Reporter | |
Comment 9•2 years ago
|
||
Hi :trickypr, just in case you missed it, the other bug has landed now.
|
|
||
Updated•2 years ago
|
|
Assignee | |
Comment 10•2 years ago
|
||
I have added a few style rules in valid-jsdoc. They can all be fixed via --fix, so I think they are worth including for consistency.
"jsdoc/check-alignment": "error",
"jsdoc/empty-tags": "error",
"jsdoc/newline-after-description": "error",
"jsdoc/no-multi-asterisks": "error",
"jsdoc/tag-lines": "error",
I should note that mozilla/require-jsdoc is much stricter than existing rules within the tree, but I think that it is more correct.
|
|
Assignee | |
Comment 11•2 years ago
|
||
Depends on D158561
|
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 12•2 years ago
|
||
Depends on D158562
|
|
||
Updated•2 years ago
|
|
|
||
Updated•2 years ago
|
|
|
Reporter | |
Updated•2 years ago
|
|
||
Comment 13•2 years ago
|
||
|
||
Comment 14•2 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 15•2 years ago
|
||
|
|
Assignee | |
Comment 16•2 years ago
|
||
Depends on D159468
|
|
Assignee | |
Comment 17•2 years ago
|
||
Depends on D159469
|
|
Assignee | |
Comment 18•2 years ago
|
||
Depends on D159470
|
|
Assignee | |
Comment 19•2 years ago
|
||
Depends on D159471
|
|
||
Comment 20•2 years ago
|
||
|
||
Comment 21•2 years ago
|
||
Awesome work!
|
||
Comment 22•2 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 23•2 years ago
|
||
The two rules here (require-jsdoc and valid-jsdoc) will likely be
removed in the not-to-distant future, so I am cleaning up their usage.
|
|
Assignee | |
Comment 24•2 years ago
|
||
Depends on D160633
|
|
Assignee | |
Comment 25•2 years ago
|
||
Depends on D160634
|
|
||
Comment 26•2 years ago
|
||
|
|
||
Comment 27•2 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 28•2 years ago
|
||
|
|
Assignee | |
Comment 29•2 years ago
|
||
Depends on D161386
|
|
Assignee | |
Comment 30•2 years ago
|
||
Depends on D161387
|
|
Assignee | |
Comment 31•2 years ago
|
||
Depends on D161388
|
|
Assignee | |
Comment 32•2 years ago
|
||
Depends on D161389
|
|
Assignee | |
Comment 33•2 years ago
|
||
Depends on D161390
|
|
Assignee | |
Comment 34•2 years ago
|
||
Depends on D161391
|
|
Assignee | |
Comment 35•2 years ago
|
||
Depends on D161392
|
|
Assignee | |
Comment 36•2 years ago
|
||
Depends on D161393
|
|
Assignee | |
Comment 37•2 years ago
|
||
That should be the last patches needed to close this bug.
|
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 38•2 years ago
|
||
Depends on D161394
|
|
||
Comment 39•2 years ago
|
||
|
|
||
Comment 40•2 years ago
|
||
|
||
Comment 41•2 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/22851c8054e9
https://hg.mozilla.org/mozilla-central/rev/cd10301fabb6
https://hg.mozilla.org/mozilla-central/rev/3cfe1f581f29
https://hg.mozilla.org/mozilla-central/rev/b7b4328002e8
https://hg.mozilla.org/mozilla-central/rev/5c01b276b5ae
|
|
Reporter | |
Comment 42•2 years ago
|
||
Oops, still have a few more to land.
|
||
Updated•2 years ago
|
|
|
||
Comment 43•2 years ago
|
||
|
||
Comment 44•2 years ago
|
||
| bugherder | ||
|
|
||
Updated•2 years ago
|
|
|
||
Comment 45•2 years ago
|
||
|
|
Reporter | |
Comment 46•2 years ago
|
||
@trickypr: Thank you for all your work on this. It has been really great to get done and a great improvement for our docs. The last patch is now in the landing process, so I'll remove the leave-open, so once this lands the bug should get automatically marked as fixed.
|
||
Comment 47•2 years ago
|
||
| bugherder | ||
|
||
Updated•2 years ago
|
|
||
Updated•2 years ago
|
Description
•