Add no-generic-link-text rule #10

khiga8 · 2022-08-26T20:59:35Z

Giving markdownlint rule-making a go 💪

Pulled out a function used in the test for the first rule into a shared helper.
Added a lint rule against generic link text in markdown called no-generic-link-text. This follows a similar rule we have in erblint-github. Seems like this would be a common violation in markdown.

For now this rule only handles inline links because I wanted to avoid the complexities around determining link text of a link tag, especially since different markdown parsers have different levels of ARIA support.

accessibility-bot · 2022-08-26T21:01:11Z

👋 Hello and thanks for pinging us! An accessibility first responder will review this soon.

💻 On PRs for our review: please provide a review environment with steps to validate, screenshots (with alt text), or videos (with description following) demonstrating functionality we should be checking. This will help speed up our review and feedback cycle.
⚠️ If this is urgent, please visit us in #accessibility on Slack and tag the first responder(s) listed in the channel topic.

test/utils/run-test.js

no-generic-link-text.js

khiga8 · 2022-12-20T20:05:55Z

I am finally revisiting this as downtime work during FR and following the steps outlined in: https://github.com/github/markdownlint-github#development to try running this rule.

However, I'm finding that the custom rules aren't running at all. I thought that this was a problem with my branch, but I'm seeing that the published package might be broken too.

In the accessibility repo, I introduced a violation but that isn't being flagged... ~~I think the way we're exporting configurations might be off. 🤔~~

Update: It seemed to be an issue with the config in accessibility repo! Proceeding!

- We don't want anything to break when link is blank. - Tests should account for links with punctuation text.

khiga8 · 2022-12-21T17:04:25Z

I made some updates because when I tested this against the github/accessibility repo, I found my logic was weird and no violations were being raised! It turns out my understanding of the parser was wrong.

I made additional updates by peeking at a link text related rules in markdownlint (md042) to make sure I was collecting the link text correctly and made my tests more robust. I then tested this against github/accessibility repo which correctly caught 3 violations. I also tested this against github/thehub and it seems to be working well.

I additionally pulled the stripAndDowncaseText out into its own file with tests.

I also added support so consumers can configure additional banned texts like so in addition to the default ones we have:

{
    "no-default-alt-text": true,
    "no-duplicate-header": true,
    "no-generic-link-text": { 
      "additional_banned_texts": ["something"]
    }
}

I opened a separate issue to add docs for each rule: #12.

I am hoping to cut a new release after this!

khiga8 · 2022-12-21T17:05:56Z

no-generic-link-text.js

+  function: function GH002(params, onError) {
+    // markdown syntax
+    const allBannedLinkTexts = bannedLinkText.concat(
+      params.config.additional_banned_texts || []


I'm using snake case to follow the convention set in markdownlint for how rule configs are set.

khiga8 · 2022-12-21T17:22:03Z

no-generic-link-text.js

+    const allBannedLinkTexts = bannedLinkText.concat(
+      params.config.additional_banned_texts || []
+    );
+    const inlineTokens = params.tokens.filter((t) => t.type === "inline");


I found markdown-it demo and using the debug functionality helpful for understanding how markdown is parsed.

Additionally, I referenced: md042.

khiga8 · 2022-12-21T17:24:57Z

I want to see this rule running in our projects successfully. Then I plan to upstream it to markdownlint. I opened an issue there and got approval (DavidAnson/markdownlint#681).

kendallgassner · 2022-12-21T17:34:52Z

test/no-generic-link-text.test.js

+      expect(results[0].errorDetail).toBe("For link: Click here");
+    });
+
+    test("additional words can be configured", async () => {


kendallgassner · 2022-12-21T17:36:25Z

no-generic-link-text.js

@@ -0,0 +1,50 @@
+const { stripAndDowncaseText } = require("./helpers/strip-and-downcase-text");
+
+const bannedLinkText = [


Would it be possible or worth making this work for various languages?

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)

I think we will want to address i18n in a separate issue! I'll make an issue.

i18n Issue!

khiga8 requested a review from a team as a code owner August 26, 2022 20:59

khiga8 requested review from alliethu and benjiallen August 26, 2022 20:59

khiga8 commented Aug 26, 2022

View reviewed changes

test/utils/run-test.js Show resolved Hide resolved

khiga8 requested review from a team, bolonio and owenniblock August 26, 2022 21:56

bolonio approved these changes Aug 29, 2022

View reviewed changes

no-generic-link-text.js Outdated Show resolved Hide resolved

owenniblock approved these changes Sep 5, 2022

View reviewed changes

khiga8 marked this pull request as draft December 20, 2022 22:41

khiga8 added 14 commits December 21, 2022 11:26

Move to helper

b64b572

Add test file

7f778c1

Add no-generic-link-text rule

def43d6

Add tests with punctuation

24071c4

Update tests and account for blank link

b908be6

- We don't want anything to break when link is blank. - Tests should account for links with punctuation text.

Ensure rule is configured

550896e

Add rule to accessibility

aa094d7

Fix tests and run linter

0f42a59

More updates

85b876e

Make tests more robust

b63e5e2

Allow additional words to be configured

4ce66a8

Run lint

cce4a05

Allow rule params to be configured

f5b1e48

Add test

7ebcdee

khiga8 force-pushed the kh-rule branch from 9bc7f50 to 7ebcdee Compare December 21, 2022 16:31

khiga8 added 3 commits December 21, 2022 11:33

Rebase conflicts

91d4422

Clean up code

1bf18ae

Move helper out and add tests

4269133

khiga8 commented Dec 21, 2022

View reviewed changes

khiga8 marked this pull request as ready for review December 21, 2022 17:08

khiga8 commented Dec 21, 2022

View reviewed changes

kendallgassner approved these changes Dec 21, 2022

View reviewed changes

khiga8 mentioned this pull request Dec 21, 2022

i18n support #13

Open

khiga8 merged commit ea1958d into main Dec 21, 2022

khiga8 deleted the kh-rule branch December 21, 2022 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add no-generic-link-text rule #10

Add no-generic-link-text rule #10

khiga8 commented Aug 26, 2022 •

edited

accessibility-bot commented Aug 26, 2022

khiga8 commented Dec 20, 2022 •

edited

khiga8 commented Dec 21, 2022 •

edited

khiga8 Dec 21, 2022 •

edited

khiga8 Dec 21, 2022

khiga8 commented Dec 21, 2022

kendallgassner Dec 21, 2022

kendallgassner Dec 21, 2022

khiga8 Dec 21, 2022

khiga8 Dec 21, 2022 •

edited

khiga8 Dec 21, 2022

		@@ -0,0 +1,50 @@
		const { stripAndDowncaseText } = require("./helpers/strip-and-downcase-text");

		const bannedLinkText = [

Add no-generic-link-text rule #10

Add no-generic-link-text rule #10

Conversation

khiga8 commented Aug 26, 2022 • edited

accessibility-bot commented Aug 26, 2022

khiga8 commented Dec 20, 2022 • edited

khiga8 commented Dec 21, 2022 • edited

khiga8 Dec 21, 2022 • edited

Choose a reason for hiding this comment

khiga8 Dec 21, 2022

Choose a reason for hiding this comment

khiga8 commented Dec 21, 2022

kendallgassner Dec 21, 2022

Choose a reason for hiding this comment

kendallgassner Dec 21, 2022

Choose a reason for hiding this comment

khiga8 Dec 21, 2022

Choose a reason for hiding this comment

khiga8 Dec 21, 2022 • edited

Choose a reason for hiding this comment

khiga8 Dec 21, 2022

Choose a reason for hiding this comment

khiga8 commented Aug 26, 2022 •

edited

khiga8 commented Dec 20, 2022 •

edited

khiga8 commented Dec 21, 2022 •

edited

khiga8 Dec 21, 2022 •

edited

khiga8 Dec 21, 2022 •

edited