fixes #1436 #1623
fixes #1436 #1623
Conversation
src/cli.js
Outdated
| @@ -81,6 +81,12 @@ export function getConfig({ useCLI = true } = {}) { | |||
| type: 'string', | |||
| requiresArg: true, | |||
| }) | |||
| .option('disable-linter-rules', { | |||
| alias: ['dlr'], | |||
There was a problem hiding this comment.
I'm not sure we need an alias for this since that option will probably used by our scripts in addons-server 99,9% of the time.
src/cli.js
Outdated
| @@ -81,6 +81,12 @@ export function getConfig({ useCLI = true } = {}) { | |||
| type: 'string', | |||
| requiresArg: true, | |||
| }) | |||
| .option('disable-linter-rules', { | |||
| alias: ['dlr'], | |||
| describe: 'Disable list of coma separated eslint rules', | |||
There was a problem hiding this comment.
This should be a comma instead of a coma
src/linter.js
Outdated
| @@ -368,6 +368,8 @@ export default class Linter { | |||
| // TODO: Bring this in line with other scanners, see: | |||
| // https://github.com/mozilla/addons-linter/issues/895 | |||
| collector: this.collector, | |||
| // list of disbled rules for js scanner | |||
src/scanners/javascript.js
Outdated
| @@ -55,7 +74,7 @@ export default class JavaScriptScanner { | |||
| parserOptions: { | |||
| ecmaVersion: 2017, | |||
| }, | |||
| rules: _ruleMapping, | |||
| rules: ruleMappingWithExclusion, | |||
There was a problem hiding this comment.
Why explicitly create a new object for that? Imho _ruleMapping should still work for this right? We essentially just want to avoid to defineRule for disabled rules.
There was a problem hiding this comment.
that's correct, my mistake, sorry
src/scanners/javascript.js
Outdated
| @@ -40,6 +57,8 @@ export default class JavaScriptScanner { | |||
| _ruleMapping = ESLINT_RULE_MAPPING, | |||
| _messages = messages, | |||
| } = {}) { | |||
| const ruleMappingWithExclusion = excludeRules(_ruleMapping, this.disabledRules); | |||
| const rulesAfterExclusion = excludeRules(_rules, this.disabledRules); | |||
There was a problem hiding this comment.
I think you could reduce the complexity of that code a bit by just using forEach with a delete inside:
import cloneDeep from 'lodash.clonedeep';
const enabledRules = cloneDeep(_rules);
this._disabledRules.forEach((key) => { delete enabledRules[key] });I think that could work (untested though)
There was a problem hiding this comment.
this._disabledRules - is object not an array
src/scanners/javascript.js
Outdated
| ...result, | ||
| [i.trim()]: true, | ||
| }; | ||
| }, {}) : {}; |
There was a problem hiding this comment.
I wonder if a simple map (given that disabledRules can be an array instead of an object, see comment down below) would work easier here.
this.disabledRules = typeof options.disabledRules === 'string' ? options.disabledRules.split(',')
.map(rule => { rule.trim() })
.filter(() => { return true; });map cleans the strings and filter filters out empty elements by default so the result should be similar.
There was a problem hiding this comment.
it's not the same in my case we have map
{
rule0: true,
rule1: true,
}
in your case we have array of rules. it just faster to look in the object if it has property, than lookup in the array if it has item, if you want I can change it to array
tests/scanners/test.javascript.js
Outdated
| expect(typeof jsScanner.disabledRules).toEqual('object'); | ||
| // This test assures us the disabledRules can be accessed like an object. | ||
| expect(typeof jsScanner.disabledRules.someUndefinedProp).toEqual('undefined'); | ||
| }); |
There was a problem hiding this comment.
I think that test should be more something like
it('should have no rules disabled by default', () => {
const jsScanner = new JavaScriptScanner('', 'filename.txt');
expect(jsScanner.disabledRules).ToEqual({}); // or [] given my comments above
});That way you can merge those two checks into one.
tests/scanners/test.javascript.js
Outdated
| expect(typeof jsScanner.disabledRules).toEqual('object'); | ||
| // This test assures us the disabledRules built properly. | ||
| expect(jsScanner.disabledRules['no-unsafe-innerhtml/no-unsafe-innerhtml']).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Same here, I would compare the actual expected disabledRule object.
tests/scanners/test.javascript.js
Outdated
| }); | ||
| expect(typeof jsScanner.disabledRules).toEqual('object'); | ||
| expect(jsScanner.disabledRules).toEqual({}); | ||
| }); |
There was a problem hiding this comment.
Ah, here you did the check correctly. I don't think you need the typeof check though.
tests/scanners/test.javascript.js
Outdated
| 'no-unsafe-innerhtml/no-unsafe-innerhtml': true, | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
That's precisely what I meant 👍
introduces new cli param
disable-linter-ruleswhich accepts coma separated list of rules to be disabled