-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add constraints to stylelint-polaris/coverage disable comments
#7589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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:') | ||
|
Member
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
Member
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:
Member
Author
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.allwould 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/coverageworked 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.disabledWarningsincludes 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 */