Add meta.docs for each rule
#246
Conversation
Eslint does support a documentation section in rule definitions. This section allows IDE plugins to display rule descriptions and directly link to the rule documentation. Details are documentad at: https://eslint.org/docs/developer-guide/working-with-rules#rule-basics
| docs: { | ||
| description: '"if ... else if" constructs should end with "else" clauses', | ||
| category: 'Best Practices', | ||
| recommended: false, |
There was a problem hiding this comment.
should it be off to be consistent with index.ts?
Can we rely on this value from docs when creating profile in index.ts? So that it's not duplicated
There was a problem hiding this comment.
should it be
offto be consistent withindex.ts?
No, it shouldn't unfortunately. Here is the type of the recommended property.
Can we rely on this value from
docswhen creating profile inindex.ts? So that it's not duplicated
Yes, we can. I will do that.
| category: 'Possible Errors', | ||
| recommended: 'error', | ||
| url: 'https://github.com/SonarSource/eslint-plugin-sonarjs/blob/master/docs/rules/no-collection-size-mischeck.md', | ||
| requiresTypeChecking: true, |
There was a problem hiding this comment.
I don't find the doc for this property. Do you know where is it coming from?
There was a problem hiding this comment.
It's coming from typescript-eslint (see Rule.ts). After looking at how it is used, we don't need it.
src/rules/no-collapsible-if.ts
Outdated
| @@ -27,6 +27,12 @@ import { report, issueLocation } from '../utils/locations'; | |||
| const rule: Rule.RuleModule = { | |||
| meta: { | |||
| type: 'suggestion', | |||
| docs: { | |||
| description: 'Collapsible "if" statements should be merged', | |||
| category: 'Stylistic Issues', | |||
There was a problem hiding this comment.
I would replace all Stylistic Issues rules in this PR with Best Practices. Stylistic Issues seems to be more about formatting.
We already have type property, so I would map suggestion to Best Practices and problem to Possible Errors
There was a problem hiding this comment.
👍 I like this mapping you are suggesting.
| @@ -44,5 +44,6 @@ it('should document all rules', () => { | |||
| existingRules.forEach(rule => { | |||
| expect(README.includes(rule)).toBe(true); | |||
| expect(fs.existsSync(`${root}/docs/rules/${rule}.md`)).toBe(true); | |||
| expect(rules[rule].meta.docs).toBeDefined(); | |||
There was a problem hiding this comment.
I suggest to assert that properties of docs are also defined and have specific values (to avoid typos)
There was a problem hiding this comment.
I think relying on the type system is enough here; it won't compile if there are any missing properties or if they don't have expected values.
suggestion -> Best Practices problem -> Possible Errors
|
Should we just rely on the file system to get the rule names ? WDYT ? eslint-plugin-sonarjs/src/index.ts Lines 22 to 28 in ef50275 |
src/rules/no-collapsible-if.ts
Outdated
| description: 'Collapsible "if" statements should be merged', | ||
| category: 'Best Practices', | ||
| recommended: 'error', | ||
| url: docsUrl('o-collapsible-if'), |
| sonarjsRules.forEach(rule => (sonarjsRuleModules[rule[0]] = require(`./rules/${rule[0]}`))); | ||
| sonarjsRules.forEach(rule => (configs.recommended.rules![`sonarjs/${rule[0]}`] = rule[1])); | ||
| sonarjsRules.forEach(rule => (sonarjsRuleModules[rule] = require(`./rules/${rule}`))); | ||
| sonarjsRules.forEach(rule => { |
There was a problem hiding this comment.
since this forEach has a full body now, we probably can merge it with the previous one
|
@yassin-kammoun-sonarsource yeah, that makes sense so adding a new rule will be easier. Also we could rely on |
|
Kudos, SonarCloud Quality Gate passed! |
No description provided.