Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 12
Add no-generic-link-text rule#10
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
b64b5727f778c1def43d624071c4b908be6550896eaa094d70f42a5985b876eb63e5e24ce66a8cce4a05f5b1e487ebcdee91d44221bf18ae4269133File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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"); | ||
ContributorAuthor 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. I found markdown-it demo and using the Additionally, I referenced: md042. | ||
| 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; | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)", | ||
| "", | ||
| ` | ||
| ## Hello | ||
| I am not a link, and unrelated. | ||
|  | ||
| `, | ||
| ]; | ||
| 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 () => { | ||
Contributor 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. 💖 | ||
| 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); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| const markdownlint = require("markdownlint"); | ||
khiga8 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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; | ||
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.
Would it be possible or worth making this work for various languages?
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.
This plugin doesn't currently have i18n support, but I added support so people can additionally configure text so they can through that!
Related: #10 (comment)
Uh oh!
There was an error while loading. Please reload this page.
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 think we will want to address i18n in a separate issue! I'll make an issue.
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.
i18n Issue!