Add report invalid scope disables #4181

vankop · 2019-07-30T10:42:45Z

Which issue, if any, is this issue related to?

Is there anything in the PR that needs further explanation?

Naive idea of collecting rules that don't exist within the configuration object is pretty straight forward - find all disabled rules that don't exist in configuration using info in disableRanges object. However, I faced with problem in tests:

/* stylelint-disable not-in-config */ a {} /* stylelint-enable */
/* stylelint-disable */ b {} /* stylelint-enable */

and vice verca

/* stylelint-disable */ b {} /* stylelint-enable */
/* stylelint-disable not-in-config */ a {} /* stylelint-enable */

In both cases rule not-in-config will be disabled twice, so invalidScopeDisables will report twice as well. I decided to enhance disableRange with 2 flags strictStart: boolean and strictEnd?: boolean, that work as describe below:

for each rule in stylelint-disable* <rule> strictStart will be true
for each rule in stylelint-enable* <rule> strictEnd will be true
for each rule in stylelint-disable* strictStart will be false and for all rule will be true
for each rule in stylelint-enable* strictEnd will be false and for all rule will be true

/* stylelint-disable not-in-config */ a {} /* stylelint-enable */ (1)
/* stylelint-disable */ b {} /* stylelint-enable */ (2)

will result in

{
    all: [{start: 2, end: 2, strictStart: true, strictEnd: true}],
    "not-in-config": [{start: 1, end: 1, strictStart: true, strictEnd: false}, {start: 2, end: 2, strictStart: false, strictEnd: false}]
}

I updated disableRanges tests, so you can find more examples there.

vankop · 2019-07-30T10:45:01Z

Actually PR looks pretty big (and it will be bigger since need to add more tests), if we ok to split it in different tickets I can do it

alexander-akait

CLI should support this option too

/cc @stylelint/core maybe we can merge all invalid comment in one option for CLI for better DX

vankop · 2019-07-30T10:58:36Z

CLI should support this option too

Thanks, forgot about cli description.

# Conflicts: # lib/cli.js

hudochenkov · 2019-07-31T11:19:27Z

Error might be caused by #4174 change. And/or this change #4151.

hudochenkov

It looks too complex for me to understand completely :) Tests are passing.

vankop · 2019-09-04T13:55:32Z

lib/assignDisabledRanges.js

+        const strict = disabledRuleName === ALL_RULES;
+
+        startDisabledRange(line, disabledRuleName, strict);
+        endDisabledRange(line, disabledRuleName, strict);


regarding to strict flag definition - if ruleName is all, then strictStart/strictEnd true, otherwise false

vankop · 2019-09-04T13:56:38Z

lib/assignDisabledRanges.js

-      startDisabledRange(line, ruleName);
-      endDisabledRange(line, ruleName);
+      startDisabledRange(line, ruleName, true);
+      endDisabledRange(line, ruleName, true);


provided ruleName is not all, e.g.:
/* stylelint-disable-line my-rule */
so strictStart/strictEnd true

vankop · 2019-09-04T13:58:24Z

lib/assignDisabledRanges.js

+          startDisabledRange(
+            comment.source.start.line,
+            ruleName,
+            ruleName === ALL_RULES


disabling all rule, so strictStart true only for all rules

vankop · 2019-09-04T13:58:56Z

lib/assignDisabledRanges.js

@@ -116,7 +140,11 @@ module.exports = function(

        Object.keys(disabledRanges).forEach(ruleName => {
          if (!_.get(_.last(disabledRanges[ruleName]), "end")) {
-            endDisabledRange(comment.source.end.line, ruleName);
+            endDisabledRange(


same for end

vankop · 2019-09-04T14:00:21Z

lib/invalidScopeDisables.js

+  const report = [];
+  const usedRules = new Set(Object.keys(config.rules || {}));
+
+  usedRules.add("all");


setup all used rules Set (include all rule)

vankop · 2019-09-04T14:01:19Z

lib/invalidScopeDisables.js

+    const disabledRules = Object.keys(rangeData);
+
+    disabledRules.forEach(rule => {
+      if (usedRules.has(rule)) {


skip creating warning for rule that exists in config (include all rule)

vankop · 2019-09-04T14:02:54Z

lib/invalidScopeDisables.js

+      }
+
+      rangeData[rule].forEach((range /*: unusedRangeT */) => {
+        if (!range.strictStart && !range.strictEnd) {


if current range is not strict from start or end, then skip it, e.g:

/* stylelint-disable */ a { color: red} /* stylelint-enable */ /* stylelint-disable color-hex */ a { color: red} /* stylelint-enable */

current code will skip creating warning for color-hex on line 1

vankop · 2019-09-04T14:04:13Z

lib/invalidScopeDisables.js

+          return;
+        }
+
+        sourceReport.ranges.push({


creating warning using report-needless-disable warnings format

vankop · 2019-09-04T14:05:10Z

@hudochenkov I have clarified some stuff

ntwb

I see no issues

hudochenkov · 2019-09-09T08:20:59Z

Added: --reportInvalidScopeDisables CLI flag (#4181).

vankop added 5 commits July 30, 2019 12:12

add reportInvalidScopeDisables flag, tests

5b224c2

add strictStart, strictEnd flags to disableRange

928f57a

fix needlessDisables regarding to new disableRanges

96779a8

fix invalidScopeDisables regarding to new disableRanges

0fa4257

add invalidScopeDisables tests, flow types fix

459259c

alexander-akait reviewed Jul 30, 2019

View reviewed changes

add cli description

6c9b9c7

vankop added 3 commits July 30, 2019 14:12

standalone-invalidScopeDisables test

e49c91d

fix tests

cdde5ca

Merge branch 'master' into add-report-invalid-scope-disables

7cb3dcb

# Conflicts: # lib/cli.js

fix tests

8d44adb

vankop mentioned this pull request Sep 3, 2019

Release 11.0.0 #4242

Closed

6 tasks

hudochenkov approved these changes Sep 4, 2019

View reviewed changes

vankop commented Sep 4, 2019

View reviewed changes

ntwb approved these changes Sep 8, 2019

View reviewed changes

hudochenkov merged commit 862b44f into stylelint:master Sep 9, 2019

vankop deleted the add-report-invalid-scope-disables branch September 9, 2019 08:52

vankop added a commit that referenced this pull request Sep 10, 2019

Merge #4181

4934cef

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add report invalid scope disables #4181

Add report invalid scope disables #4181

vankop commented Jul 30, 2019 •

edited

vankop commented Jul 30, 2019

alexander-akait left a comment

vankop commented Jul 30, 2019

hudochenkov commented Jul 31, 2019 •

edited

hudochenkov left a comment

vankop Sep 4, 2019

vankop Sep 4, 2019 •

edited

vankop Sep 4, 2019

vankop Sep 4, 2019

vankop Sep 4, 2019

vankop Sep 4, 2019

vankop Sep 4, 2019 •

edited

vankop Sep 4, 2019

vankop commented Sep 4, 2019

ntwb left a comment

hudochenkov commented Sep 9, 2019

Add report invalid scope disables #4181

Add report invalid scope disables #4181

Conversation

vankop commented Jul 30, 2019 • edited

vankop commented Jul 30, 2019

alexander-akait left a comment

Choose a reason for hiding this comment

vankop commented Jul 30, 2019

hudochenkov commented Jul 31, 2019 • edited

hudochenkov left a comment

Choose a reason for hiding this comment

vankop Sep 4, 2019

Choose a reason for hiding this comment

vankop Sep 4, 2019 • edited

Choose a reason for hiding this comment

vankop Sep 4, 2019

Choose a reason for hiding this comment

vankop Sep 4, 2019

Choose a reason for hiding this comment

vankop Sep 4, 2019

Choose a reason for hiding this comment

vankop Sep 4, 2019

Choose a reason for hiding this comment

vankop Sep 4, 2019 • edited

Choose a reason for hiding this comment

vankop Sep 4, 2019

Choose a reason for hiding this comment

vankop commented Sep 4, 2019

ntwb left a comment

Choose a reason for hiding this comment

hudochenkov commented Sep 9, 2019

vankop commented Jul 30, 2019 •

edited

hudochenkov commented Jul 31, 2019 •

edited

vankop Sep 4, 2019 •

edited

vankop Sep 4, 2019 •

edited