-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add constraints to stylelint-polaris/coverage
disable comments
#7589
Changes from all commits
a1ff92f
b142038
5c8b1ba
3b97ef0
bbe02d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@shopify/stylelint-polaris': patch | ||
--- | ||
|
||
Add constraints to `stylelint-polaris/coverage` disable comments |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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,84 @@ 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:') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Love this. I do want to make sure we are aligned with the Not against There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great points. We could change it if needed in a future major and have overlap of multiply supported prefixes. So maybe we start with Just some alternatives prefixes:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, great callout! My main focus in this iteration was ensuring we can capture disabled coverage rules and validate some form of context was provided for tracking. I landed on |
||
) { | ||
continue; | ||
} | ||
|
||
const stylelintDisableText = disabledRange.comment.text | ||
.split('--')[0] | ||
.trim(); | ||
|
||
stylelint.utils.report({ | ||
message: `Expected /* ${stylelintDisableText} -- polaris: Reason for disabling */`, | ||
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, | ||
); | ||
} | ||
|
||
/** | ||
* 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) { | ||
if ( | ||
!disabledRange.comment.text.startsWith('stylelint-disable-next-line') && | ||
!isNumber(disabledRange.end) | ||
) { | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
function validatePrimaryOptions(primaryOptions) { | ||
if (!isObject(primaryOptions)) return false; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought only targeting the
disabledRanges.all
would be an issue if a specific rule name was used; however, because of how we are creating the rules dynamically, Stylelint won't let you target it since it isn't present as a static rule in the config 🤯 brilliantThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, leveraging

stylelint-polaris/coverage
worked out well! Because we categorize rules with custom names and check them manually, I was able to easily filter out disabled coverage rules fromdisabledRanges.all
:Notice the above
result.stylelint.disabledWarnings
includes the category that was disabled e.g./motion
. This can be leveraged in future iterations to require authors define each disabled category e.g./* stylelint-disable -- p-colors, p-motion: context */