From a1ff92f19bffae40cc5290eb8d1ffe69aea333a9 Mon Sep 17 00:00:00 2001 From: Aaron Casanova <32409546+aaronccasanova@users.noreply.github.com> Date: Mon, 31 Oct 2022 15:02:58 -0700 Subject: [PATCH 1/5] Add constraints to stylelint-polaris/coverage disable comments --- stylelint-polaris/plugins/coverage/index.js | 71 ++++++++++++++++++- .../plugins/coverage/index.test.js | 54 ++++++++++++++ 2 files changed, 124 insertions(+), 1 deletion(-) diff --git a/stylelint-polaris/plugins/coverage/index.js b/stylelint-polaris/plugins/coverage/index.js index e755dc125a5..df47501ef65 100644 --- a/stylelint-polaris/plugins/coverage/index.js +++ b/stylelint-polaris/plugins/coverage/index.js @@ -1,6 +1,6 @@ const stylelint = require('stylelint'); -const {isObject} = require('../../utils'); +const {isObject, isNumber} = require('../../utils'); const ruleName = 'stylelint-polaris/coverage'; @@ -55,10 +55,79 @@ module.exports = stylelint.createPlugin( }, ); } + + const disabledCoverageWarnings = + result.stylelint.disabledWarnings?.filter((disabledWarning) => + disabledWarning.rule.startsWith(ruleName), + ); + + if (!disabledCoverageWarnings?.length) return; + + const disabledCoverageLines = Array.from( + new Set(disabledCoverageWarnings.map(({line}) => line)), + ); + + // Ensure all stylelint-polaris/coverage disable comments + // have a description prefixed with "polaris:" + for (const disabledRange of result.stylelint.disabledRanges.all) { + if ( + !isDisabledCoverageRule(disabledCoverageLines, disabledRange) || + disabledRange.description?.startsWith('polaris:') + ) { + continue; + } + + stylelint.utils.report({ + message: `Missing "polaris:" prefix in disable comment description`, + ruleName, + result, + node: disabledRange.comment, + // Note: `stylelint-disable` comments (without next-line) appear to + // be special cased in that they do not trigger warnings when reported. + // Setting `line` to an invalid line number forces the warning to be + // reported and the above comment `node` is used to display the + // location information: + // https://github.com/stylelint/stylelint/blob/57cbcd4eb0ee809006a1e3d2ccfe73af48744ad5/lib/utils/report.js#L49-L52 + line: -1, + }); + } }; }, ); +/** + * @param {number[]} disabledCoverageLines + * @param {import('stylelint').DisabledRange} disabledRange + */ +function isDisabledCoverageRule(disabledCoverageLines, disabledRange) { + if (isUnclosedDisabledRange(disabledRange)) { + return disabledCoverageLines.some( + (disabledCoverageLine) => disabledCoverageLine >= disabledRange.start, + ); + } + + return disabledCoverageLines.some( + (coverageDisabledLine) => + coverageDisabledLine >= disabledRange.start && + coverageDisabledLine <= disabledRange.end, + ); +} + +/** + * Check if the disabled range includes both stylelint-disable and stylelint-enable comments + * @param {import('stylelint').DisabledRange} disabledRange + */ +function isUnclosedDisabledRange(disabledRange) { + if ( + disabledRange.comment.text === 'stylelint-disable' && + !isNumber(disabledRange.end) + ) { + return true; + } + + return false; +} + function validatePrimaryOptions(primaryOptions) { if (!isObject(primaryOptions)) return false; diff --git a/stylelint-polaris/plugins/coverage/index.test.js b/stylelint-polaris/plugins/coverage/index.test.js index 4f8973f0d35..1407aa3792b 100644 --- a/stylelint-polaris/plugins/coverage/index.test.js +++ b/stylelint-polaris/plugins/coverage/index.test.js @@ -16,6 +16,31 @@ testRule({ code: '@media (min-width: 320px) {}', description: 'Uses allowed at-rule', }, + { + code: ` + /* stylelint-disable -- polaris: context */ + @keyframes foo {} + /* stylelint-enable */ + `, + description: + 'Uses disallowed at-rule with disable/enable comment and context', + }, + { + code: ` + /* stylelint-disable -- polaris: context */ + @keyframes foo {} + `, + description: + 'Uses disallowed at-rule with disable comment and context and without enable comment', + }, + { + code: ` + /* stylelint-disable-next-line -- polaris: context */ + @keyframes foo {} + `, + description: + 'Uses disallowed at-rule with disable next line comment and context', + }, ], reject: [ @@ -25,5 +50,34 @@ testRule({ message: 'Unexpected at-rule "keyframes" (stylelint-polaris/coverage/motion)', }, + { + code: ` + /* stylelint-disable */ + @keyframes foo {} + /* stylelint-enable */ + `, + description: + 'Uses disallowed at-rule with disable/enable comment and without context', + message: 'Missing "polaris:" prefix in disable comment description', + }, + { + code: ` + /* stylelint-disable */ + + @keyframes fooz {} + `, + description: + 'Uses disallowed at-rule with disable comment and without context and enable comment', + message: 'Missing "polaris:" prefix in disable comment description', + }, + { + code: ` + /* stylelint-disable-next-line */ + @keyframes foo {} + `, + description: + 'Uses disallowed at-rule with disable next line comment and without context', + message: 'Missing "polaris:" prefix in disable comment description', + }, ], }); From b142038ec80eabfc3e6822a53c715f012ed202c3 Mon Sep 17 00:00:00 2001 From: Aaron Casanova <32409546+aaronccasanova@users.noreply.github.com> Date: Mon, 31 Oct 2022 16:58:02 -0700 Subject: [PATCH 2/5] Add changeset entry --- .changeset/tidy-cougars-talk.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tidy-cougars-talk.md diff --git a/.changeset/tidy-cougars-talk.md b/.changeset/tidy-cougars-talk.md new file mode 100644 index 00000000000..502d754b84d --- /dev/null +++ b/.changeset/tidy-cougars-talk.md @@ -0,0 +1,5 @@ +--- +'@shopify/stylelint-polaris': patch +--- + +Add constraints to `stylelint-polaris/coverage` disable comments From 5c8b1bae262a7914d91d98a3012923aa9729b40b Mon Sep 17 00:00:00 2001 From: Aaron Casanova <32409546+aaronccasanova@users.noreply.github.com> Date: Thu, 3 Nov 2022 10:45:37 -0700 Subject: [PATCH 3/5] Improve stylelint-disable comment error message --- stylelint-polaris/plugins/coverage/index.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/stylelint-polaris/plugins/coverage/index.js b/stylelint-polaris/plugins/coverage/index.js index df47501ef65..493a31ef8b2 100644 --- a/stylelint-polaris/plugins/coverage/index.js +++ b/stylelint-polaris/plugins/coverage/index.js @@ -77,8 +77,12 @@ module.exports = stylelint.createPlugin( continue; } + const stylelintDisableText = disabledRange.comment.text + .split('--')[0] + .trim(); + stylelint.utils.report({ - message: `Missing "polaris:" prefix in disable comment description`, + message: `Expected /* ${stylelintDisableText} -- polaris: Context comment for disable */`, ruleName, result, node: disabledRange.comment, @@ -119,7 +123,7 @@ function isDisabledCoverageRule(disabledCoverageLines, disabledRange) { */ function isUnclosedDisabledRange(disabledRange) { if ( - disabledRange.comment.text === 'stylelint-disable' && + !disabledRange.comment.text.startsWith('stylelint-disable-next-line') && !isNumber(disabledRange.end) ) { return true; From 3b97ef0fa17f651f65f85be8d267c05dead54aed Mon Sep 17 00:00:00 2001 From: Aaron Casanova <32409546+aaronccasanova@users.noreply.github.com> Date: Thu, 3 Nov 2022 11:08:04 -0700 Subject: [PATCH 4/5] Further refine disable comment error message --- stylelint-polaris/plugins/coverage/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stylelint-polaris/plugins/coverage/index.js b/stylelint-polaris/plugins/coverage/index.js index 493a31ef8b2..7495ddea50a 100644 --- a/stylelint-polaris/plugins/coverage/index.js +++ b/stylelint-polaris/plugins/coverage/index.js @@ -82,7 +82,7 @@ module.exports = stylelint.createPlugin( .trim(); stylelint.utils.report({ - message: `Expected /* ${stylelintDisableText} -- polaris: Context comment for disable */`, + message: `Expected /* ${stylelintDisableText} -- polaris: Reason for disabling */`, ruleName, result, node: disabledRange.comment, From bbe02d8a34aee0654d40c9aeab8e527600fbc92b Mon Sep 17 00:00:00 2001 From: Aaron Casanova <32409546+aaronccasanova@users.noreply.github.com> Date: Thu, 3 Nov 2022 11:25:47 -0700 Subject: [PATCH 5/5] Update test error messages and isUnclosedDisabledRange description --- stylelint-polaris/plugins/coverage/index.js | 3 ++- stylelint-polaris/plugins/coverage/index.test.js | 9 ++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/stylelint-polaris/plugins/coverage/index.js b/stylelint-polaris/plugins/coverage/index.js index 7495ddea50a..5423352a398 100644 --- a/stylelint-polaris/plugins/coverage/index.js +++ b/stylelint-polaris/plugins/coverage/index.js @@ -118,7 +118,8 @@ function isDisabledCoverageRule(disabledCoverageLines, disabledRange) { } /** - * Check if the disabled range includes both stylelint-disable and stylelint-enable comments + * Checks if the `disabledRange` is an unclosed `stylelint-disable` comment + * e.g. The `stylelint-disable` comment is NOT followed by `stylelint-enable` * @param {import('stylelint').DisabledRange} disabledRange */ function isUnclosedDisabledRange(disabledRange) { diff --git a/stylelint-polaris/plugins/coverage/index.test.js b/stylelint-polaris/plugins/coverage/index.test.js index 1407aa3792b..dc82f11b66d 100644 --- a/stylelint-polaris/plugins/coverage/index.test.js +++ b/stylelint-polaris/plugins/coverage/index.test.js @@ -58,7 +58,8 @@ testRule({ `, description: 'Uses disallowed at-rule with disable/enable comment and without context', - message: 'Missing "polaris:" prefix in disable comment description', + message: + 'Expected /* stylelint-disable -- polaris: Reason for disabling */', }, { code: ` @@ -68,7 +69,8 @@ testRule({ `, description: 'Uses disallowed at-rule with disable comment and without context and enable comment', - message: 'Missing "polaris:" prefix in disable comment description', + message: + 'Expected /* stylelint-disable -- polaris: Reason for disabling */', }, { code: ` @@ -77,7 +79,8 @@ testRule({ `, description: 'Uses disallowed at-rule with disable next line comment and without context', - message: 'Missing "polaris:" prefix in disable comment description', + message: + 'Expected /* stylelint-disable-next-line -- polaris: Reason for disabling */', }, ], });