Append stylelint rule name to coverage category rule name; only allow…

… string custom messages; update tests
Shopify · Dec 8, 2022 · 5059cc7 · 5059cc7
1 parent 22f6917
commit 5059cc7
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 43 deletions.
diff --git a/stylelint-polaris/index.js b/stylelint-polaris/index.js
@@ -7,13 +7,13 @@ const {
 /**
  * @type {import('./plugins/coverage').CategorizedRules} The stylelint-polaris/coverage rule expects a 3-dimensional rule config that groups Stylelint rules by coverage categories. It reports problems with dynamic rule names by appending the category to the coverage plugin's rule name
 
-(e.g., Unexpected named color "blue" (color-named) Please use a Polaris color token Stylelint(stylelint-polaris/coverage/colors")
+(e.g., Unexpected named color "blue" - Please use a Polaris color token Stylelint(stylelint-polaris/coverage/colors/color-named)")
 */
 const stylelintPolarisCoverageOptions = {
   colors: [
     {
       'color-named': 'never',
-      'color-no-hex': true,
+      'color-no-hex': [true, {message: 'Please use a Polaris color token'}],
       'scss/function-color-relative': true,
       'declaration-property-value-disallowed-list': {
         opacity: [/(?!0|1)\d$|^\d{2,}|^[1-9]+\.|^\d+\.\d+\.|^0\.\d{3,}/],

diff --git a/stylelint-polaris/plugins/coverage/index.js b/stylelint-polaris/plugins/coverage/index.js
@@ -1,11 +1,12 @@
 const stylelint = require('stylelint');
 
-const {isPlainObject, getMessageArgs} = require('../../utils');
+const {isPlainObject} = require('../../utils');
 
 const coverageRuleName = 'stylelint-polaris/coverage';
 
 /**
  * @typedef {import('stylelint').ConfigRules} StylelintRuleConfig
+ * @property {string} message - Message appended to the warning in place of the default category message
  */
 
 /**
@@ -27,7 +28,7 @@ const coverageRuleName = 'stylelint-polaris/coverage';
 // https://github.com/stylelint/stylelint/blob/57cbcd4eb0ee809006a1e3d2ccfe73af48744ad5/lib/utils/report.js#L49-L52
 const forceReport = {line: -1};
 const defaultMeta = {
-  url: 'https://github.com/Shopify/polaris/tree/main/stylelint-polaris/plugins/coverage/README.md',
+  url: 'https://github.com/Shopify/polaris/tree/main/stylelint-polaris/plugins/coverage/README.md#rules',
 };
 
 module.exports = stylelint.createPlugin(
@@ -47,13 +48,13 @@ module.exports = stylelint.createPlugin(
         stylelintRules,
       )) {
         coverageRules.push({
-          ruleName: `${coverageRuleName}/${category}`,
+          ruleName: `${coverageRuleName}/${category}/${stylelintRuleName}`,
           stylelintRuleName,
           ruleSettings: stylelintRuleConfig,
           severity: stylelintRuleConfig?.[1]?.severity,
           fix: context.fix && !stylelintRuleConfig?.[1]?.disableFix,
-          customMessage: stylelintRuleConfig?.[1]?.message,
-          appendedMessage: categorySettings?.message,
+          appendedMessage:
+            stylelintRuleConfig?.[1]?.message || categorySettings?.message,
           meta: categorySettings?.meta || defaultMeta,
         });
       }
@@ -77,7 +78,6 @@ module.exports = stylelint.createPlugin(
           ruleSettings,
           fix,
           meta,
-          customMessage = '',
           appendedMessage = '',
           severity = result.stylelint.config?.defaultSeverity,
         } = rule;
@@ -91,24 +91,23 @@ module.exports = stylelint.createPlugin(
             result,
           },
           (warning) => {
-            // We insert the meta for the rules on the stylelint result, because the rules are reported with dynamic rule names instead of each category being its own plugin. See Stylelint issue for context: https://github.com/stylelint/stylelint/issues/6513
-            result.stylelint.ruleMetadata[ruleName] = meta;
+            // We insert the meta for the rules on the stylelint result directly, because the rules are reported with dynamic rule names instead of each category being its own plugin to reduce duplicate code. See Stylelint issue for context: https://github.com/stylelint/stylelint/issues/6513
+            result.stylelint.ruleMetadata[ruleName] ||= meta;
 
-            const defaultMessage = appendedMessage
-              ? `${warning.text} ${appendedMessage}`
-              : warning.text;
+            const warningMessage = warning.text.substring(
+              0,
+              warning.text.indexOf(` (${stylelintRuleName})`),
+            );
 
-            const messageArgs =
-              typeof customMessage === 'function'
-                ? getMessageArgs(stylelintRuleName, warning.node)
-                : undefined;
+            const message = appendedMessage
+              ? `${warningMessage} - ${appendedMessage}`
+              : warningMessage;
 
             stylelint.utils.report({
               result,
               ruleName,
               meta,
-              messageArgs,
-              message: customMessage || defaultMessage,
+              message,
               severity: severity || 'error',
               // If `warning.node` is NOT present, the warning is
               // referring to a misconfigured rule

diff --git a/stylelint-polaris/plugins/coverage/index.test.js b/stylelint-polaris/plugins/coverage/index.test.js
@@ -6,16 +6,13 @@ const config = {
   colors: [
     {
       'color-named': 'never',
-      'color-no-hex': [true, {message: 'Overridden category rule message'}],
-      'function-disallowed-list': [
-        'rgb',
-        {
-          message: (func) => `Overridden category rule message "${func}"`,
-        },
+      'color-no-hex': [
+        true,
+        {message: 'Appended Stylelint rule config message'},
       ],
     },
     {
-      message: 'Appended category rule message',
+      message: 'Appended category rule config message',
     },
   ],
   motion: {
@@ -43,29 +40,24 @@ testRule({
     {
       code: '.class {color: #bad;}',
       description: 'Overrides appended category rule warning text (string)',
-      message: 'Overridden category rule message',
+      message:
+        'Unexpected hex color "#bad" - Appended Stylelint rule config message',
     },
     {
       code: '.class {color: red;}',
       description: 'Appends message to category rule warning text',
       message:
-        'Unexpected named color "red" (color-named) Appended category rule message',
-    },
-    {
-      code: '.class {color: rgb(0, 256, 0);}',
-      description: 'Overrides appended category rule warning text (function)',
-      message: 'Overridden category rule message "rgb(0, 256, 0)"',
+        'Unexpected named color "red" - Appended category rule config message',
     },
     {
       code: '@keyframes foo {}',
       description: 'Uses disallowed at-rule (built-in rule)',
-      message: 'Unexpected at-rule "keyframes" (at-rule-disallowed-list)',
+      message: 'Unexpected at-rule "keyframes"',
     },
     {
       code: '.class {color: var(--p-legacy-var);}',
       description: 'Uses disallowed legacy variable (custom rule)',
-      message:
-        'Unexpected disallowed value "--p-legacy-var" (stylelint-polaris/global-disallowed-list)',
+      message: 'Unexpected disallowed value "--p-legacy-var"',
     },
   ],
 });
diff --git a/stylelint-polaris/plugins/custom-properties-allowed-list/index.js b/stylelint-polaris/plugins/custom-properties-allowed-list/index.js
@@ -21,7 +21,7 @@ const messages = stylelint.utils.ruleMessages(ruleName, {
       : null;
 
     const invalidValueMessage = invalidValue
-      ? `Unexpected value "${invalidValue}" for custom property "${invalidProperty}"`
+      ? `Unexpected value "var(${invalidValue})" for property "${invalidProperty}"`
       : null;
 
     return [invalidPropertyMessage, invalidValueMessage]

diff --git a/stylelint-polaris/plugins/global-disallowed-list/index.test.js b/stylelint-polaris/plugins/global-disallowed-list/index.test.js
@@ -1,6 +1,6 @@
-const {messages, ruleName} = require('.');
+const {ruleName} = require('.');
 
-const config = [[/rem\(/, /--p-button-font/]];
+const config = [[/\$font-size-data/, /--p-button-font/]];
 
 testRule({
   ruleName,
@@ -16,18 +16,20 @@ testRule({
 
   reject: [
     {
-      code: '.a { font-size: rem(20px); }',
+      code: '.a { font-size: $font-size-data; }',
       description: 'Uses something on the disallowed list',
-      message: messages.rejected('rem('),
+      message:
+        'Unexpected disallowed value "$font-size-data" (stylelint-polaris/global-disallowed-list)',
       line: 1,
       column: 17,
       endLine: 1,
-      endColumn: 21,
+      endColumn: 32,
     },
     {
       code: '.a { color: var(--p-button-font); }',
       description: 'Uses something on the disallowed list',
-      message: messages.rejected('--p-button-font'),
+      message:
+        'Unexpected disallowed value "--p-button-font" (stylelint-polaris/global-disallowed-list)',
       line: 1,
       column: 17,
       endLine: 1,