Add no-generic-link-text rule (#10)

* Move to helper * Add test file * Add no-generic-link-text rule * Add tests with punctuation * Update tests and account for blank link - We don't want anything to break when link is blank. - Tests should account for links with punctuation text. * Ensure rule is configured * Add rule to accessibility * Fix tests and run linter * More updates * Make tests more robust * Allow additional words to be configured * Run lint * Allow rule params to be configured * Add test * Rebase conflicts * Clean up code * Move helper out and add tests
github · Dec 21, 2022 · ea1958d · ea1958d
1 parent 18247e8
commit ea1958d
Show file tree

Hide file tree

Showing 9 changed files with 203 additions and 38 deletions.
diff --git a/helpers/strip-and-downcase-text.js b/helpers/strip-and-downcase-text.js
@@ -0,0 +1,10 @@
+/* Downcase and strip extra whitespaces and punctuation */
+function stripAndDowncaseText(text) {
+  return text
+    .toLowerCase()
+    .replace(/[.,/#!$%^&*;:{}=\-_`~()]/g, "")
+    .replace(/\s+/g, " ")
+    .trim();
+}
+
+module.exports = { stripAndDowncaseText };
diff --git a/index.js b/index.js
@@ -3,8 +3,9 @@ const _ = require("lodash");
 const accessibilityRules = require("./style/accessibility.json");
 const base = require("./style/base.json");
 const noDefaultAltText = require("./no-default-alt-text");
+const noGenericLinkText = require("./no-generic-link-text");
 
-const customRules = [noDefaultAltText];
+const customRules = [noDefaultAltText, noGenericLinkText];
 
 module.exports = [...customRules];
 

diff --git a/no-generic-link-text.js b/no-generic-link-text.js
@@ -0,0 +1,50 @@
+const { stripAndDowncaseText } = require("./helpers/strip-and-downcase-text");
+
+const bannedLinkText = [
+  "read more",
+  "learn more",
+  "more",
+  "here",
+  "click here",
+  "link",
+];
+
+module.exports = {
+  names: ["GH002", "no-generic-link-text"],
+  description:
+    "Avoid using generic link text like `Learn more` or `Click here`",
+  information: new URL(
+    "https://primer.style/design/accessibility/links#writing-link-text"
+  ),
+  tags: ["accessibility", "links"],
+  function: function GH002(params, onError) {
+    // markdown syntax
+    const allBannedLinkTexts = bannedLinkText.concat(
+      params.config.additional_banned_texts || []
+    );
+    const inlineTokens = params.tokens.filter((t) => t.type === "inline");
+    for (const token of inlineTokens) {
+      const { children } = token;
+      let inLink = false;
+      let linkText = "";
+
+      for (const child of children) {
+        const { content, type } = child;
+        if (type === "link_open") {
+          inLink = true;
+          linkText = "";
+        } else if (type === "link_close") {
+          inLink = false;
+          if (allBannedLinkTexts.includes(stripAndDowncaseText(linkText))) {
+            onError({
+              lineNumber: child.lineNumber,
+              detail: `For link: ${linkText}`,
+            });
+          }
+        } else if (inLink) {
+          linkText += content;
+        }
+      }
+    }
+  },
+};
diff --git a/style/accessibility.json b/style/accessibility.json
@@ -2,6 +2,7 @@
     "no-default-alt-text": true,
     "no-duplicate-header": true,
     "no-emphasis-as-header": true,
+    "no-generic-link-text": true,
     "no-space-in-links": false,
     "ol-prefix": "ordered",
     "single-h1": true,

diff --git a/test/helpers/strip-and-downcase-text.test.js b/test/helpers/strip-and-downcase-text.test.js
@@ -0,0 +1,20 @@
+const {
+  stripAndDowncaseText,
+} = require("../../helpers/strip-and-downcase-text");
+
+describe("stripAndDowncaseText", () => {
+  test("strips extra whitespace", () => {
+    expect(stripAndDowncaseText(" read  more  ")).toBe("read more");
+    expect(stripAndDowncaseText(" learn    ")).toBe("learn");
+  });
+
+  test("strips punctuation", () => {
+    expect(stripAndDowncaseText("learn more!!!!")).toBe("learn more");
+    expect(stripAndDowncaseText("I like dogs...")).toBe("i like dogs");
+  });
+
+  test("downcases text", () => {
+    expect(stripAndDowncaseText("HeRe")).toBe("here");
+    expect(stripAndDowncaseText("CLICK HERE")).toBe("click here");
+  });
+});
diff --git a/test/no-default-alt-text.test.js b/test/no-default-alt-text.test.js
@@ -1,33 +1,5 @@
-const markdownlint = require("markdownlint");
 const altTextRule = require("../no-default-alt-text");
-
-const thisRuleName = altTextRule.names[1];
-
-const config = {
-  config: {
-    default: false,
-    [thisRuleName]: true,
-  },
-  customRules: [altTextRule],
-};
-
-async function runTest(strings) {
-  return await Promise.all(
-    strings.map((variation) => {
-      const thisTestConfig = {
-        ...config,
-        strings: [variation],
-      };
-
-      return new Promise((resolve, reject) => {
-        markdownlint(thisTestConfig, (err, result) => {
-          if (err) reject(err);
-          resolve(result[0][0]);
-        });
-      });
-    })
-  );
-}
+const runTest = require("./utils/run-test").runTest;
 
 describe("GH001: No Default Alt Text", () => {
   describe("successes", () => {
@@ -36,7 +8,7 @@ describe("GH001: No Default Alt Text", () => {
         "![Chart with a single root node reading 'Example'](https://user-images.githubusercontent.com/abcdef.png)",
       ];
 
-      const results = await runTest(strings);
+      const results = await runTest(strings, altTextRule);
 
       for (const result of results) {
         expect(result).not.toBeDefined();
@@ -47,7 +19,7 @@ describe("GH001: No Default Alt Text", () => {
         '<img alt="A helpful description" src="https://user-images.githubusercontent.com/abcdef.png">',
       ];
 
-      const results = await runTest(strings);
+      const results = await runTest(strings, altTextRule);
 
       for (const result of results) {
         expect(result).not.toBeDefined();
@@ -63,7 +35,7 @@ describe("GH001: No Default Alt Text", () => {
         "![Screenshot 2022-06-26 at 7 41 30 PM](https://user-images.githubusercontent.com/abcdef.png)",
       ];
 
-      const results = await runTest(strings);
+      const results = await runTest(strings, altTextRule);
 
       const failedRules = results
         .map((result) => result.ruleNames)
@@ -72,7 +44,7 @@ describe("GH001: No Default Alt Text", () => {
 
       expect(failedRules).toHaveLength(4);
       for (const rule of failedRules) {
-        expect(rule).toBe(thisRuleName);
+        expect(rule).toBe("no-default-alt-text");
       }
     });
 
@@ -84,7 +56,7 @@ describe("GH001: No Default Alt Text", () => {
         '<img alt="Screenshot 2022-06-26 at 7 41 30 PM" src="https://user-images.githubusercontent.com/abcdef.png">',
       ];
 
-      const results = await runTest(strings);
+      const results = await runTest(strings, altTextRule);
 
       const failedRules = results
         .map((result) => result.ruleNames)
@@ -93,7 +65,7 @@ describe("GH001: No Default Alt Text", () => {
 
       expect(failedRules).toHaveLength(4);
       for (const rule of failedRules) {
-        expect(rule).toBe(thisRuleName);
+        expect(rule).toBe("no-default-alt-text");
       }
     });
 
@@ -103,7 +75,7 @@ describe("GH001: No Default Alt Text", () => {
         '<img alt="Screen Shot 2022-06-26 at 7 41 30 PM" src="https://user-images.githubusercontent.com/abcdef.png">',
       ];
 
-      const results = await runTest(strings);
+      const results = await runTest(strings, altTextRule);
 
       expect(results[0].ruleDescription).toMatch(
         /Images should not use the MacOS default screenshot filename as alternate text/

diff --git a/test/no-generic-link-text.test.js b/test/no-generic-link-text.test.js
@@ -0,0 +1,79 @@
+const noGenericLinkTextRule = require("../no-generic-link-text");
+const runTest = require("./utils/run-test").runTest;
+
+describe("GH002: No Generic Link Text", () => {
+  describe("successes", () => {
+    test("inline", async () => {
+      const strings = [
+        "[GitHub](https://www.github.com)",
+        "[Read more about GitHub](https://www.github.com/about)",
+        "[](www.github.com)",
+        "![Image](www.github.com)",
+        `
+        ## Hello
+        I am not a link, and unrelated.
+        ![GitHub](some_image.png)
+        `,
+      ];
+
+      const results = await runTest(strings, noGenericLinkTextRule);
+
+      for (const result of results) {
+        expect(result).not.toBeDefined();
+      }
+    });
+  });
+  describe("failures", () => {
+    test("inline", async () => {
+      const strings = [
+        "[Click here](www.github.com)",
+        "[here](www.github.com)",
+        "Please [read more](www.github.com)",
+        "[more](www.github.com)",
+        "[link](www.github.com)",
+        "You may [learn more](www.github.com) at GitHub",
+        "[learn more.](www.github.com)",
+        "[click here!](www.github.com)",
+      ];
+
+      const results = await runTest(strings, noGenericLinkTextRule);
+
+      const failedRules = results
+        .map((result) => result.ruleNames)
+        .flat()
+        .filter((name) => !name.includes("GH"));
+
+      expect(failedRules).toHaveLength(8);
+      for (const rule of failedRules) {
+        expect(rule).toBe("no-generic-link-text");
+      }
+    });
+
+    test("error message", async () => {
+      const strings = ["[Click here](www.github.com)"];
+
+      const results = await runTest(strings, noGenericLinkTextRule);
+
+      expect(results[0].ruleDescription).toMatch(
+        /Avoid using generic link text like `Learn more` or `Click here`/
+      );
+      expect(results[0].errorDetail).toBe("For link: Click here");
+    });
+
+    test("additional words can be configured", async () => {
+      const results = await runTest(
+        ["[something](www.github.com)"],
+        noGenericLinkTextRule,
+        // eslint-disable-next-line camelcase
+        { additional_banned_texts: ["something"] }
+      );
+
+      const failedRules = results
+        .map((result) => result.ruleNames)
+        .flat()
+        .filter((name) => !name.includes("GH"));
+
+      expect(failedRules).toHaveLength(1);
+    });
+  });
+});
diff --git a/test/usage.test.js b/test/usage.test.js
@@ -4,7 +4,7 @@ describe("usage", () => {
   describe("default export", () => {
     test("custom rules on default export", () => {
       const rules = githubMarkdownLint;
-      expect(rules).toHaveLength(1);
+      expect(rules).toHaveLength(2);
       expect(rules[0].names).toEqual(["GH001", "no-default-alt-text"]);
     });
   });
@@ -17,6 +17,7 @@ describe("usage", () => {
         "no-space-in-links": false,
         "single-h1": true,
         "no-emphasis-as-header": true,
+        "no-generic-link-text": true,
         "ul-style": true,
         default: true,
         "no-inline-html": false,

diff --git a/test/utils/run-test.js b/test/utils/run-test.js
@@ -0,0 +1,31 @@
+const markdownlint = require("markdownlint");
+
+async function runTest(strings, rule, ruleConfig) {
+  const thisRuleName = rule.names[1];
+
+  const config = {
+    config: {
+      default: false,
+      [thisRuleName]: ruleConfig || true,
+    },
+    customRules: [rule],
+  };
+
+  return await Promise.all(
+    strings.map((variation) => {
+      const thisTestConfig = {
+        ...config,
+        strings: [variation],
+      };
+
+      return new Promise((resolve, reject) => {
+        markdownlint(thisTestConfig, (err, result) => {
+          if (err) reject(err);
+          resolve(result[0][0]);
+        });
+      });
+    })
+  );
+}
+
+exports.runTest = runTest;