Skip to content
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

[WIP] Add new rule: no-generic-link-name #1459

Draft
wants to merge 39 commits into
base: next
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
a1c000f
Start doc page for new rule
khiga8 Jan 6, 2025
ddf030e
Add rule and test
khiga8 Jan 6, 2025
0a60d3d
Move validLink check to micromark helper
khiga8 Jan 6, 2025
b575b03
Add documentation for new rule
khiga8 Jan 6, 2025
443b77c
Add downcase and strip helper function
khiga8 Jan 6, 2025
9cee2db
Update all tests so they pass
khiga8 Jan 6, 2025
9fbf423
Update tests
khiga8 Jan 6, 2025
47db690
update rule description
khiga8 Jan 6, 2025
2b93c7e
update snapshots
khiga8 Jan 6, 2025
e141567
Rename no-generic-link-name to descriptive-link-text
khiga8 Jan 7, 2025
b93cfef
Move and rename downcase method.
khiga8 Jan 7, 2025
bbac469
Remove validLink helper
khiga8 Jan 7, 2025
bfbcadd
Address more stylistic comments
khiga8 Jan 7, 2025
7b0467b
Remove no longer used
khiga8 Jan 7, 2025
6be4eea
Address more comments
khiga8 Jan 7, 2025
6386afe
Update schema and snapshots
khiga8 Jan 7, 2025
3a5e27f
Auto-generate docs
khiga8 Jan 7, 2025
da24423
Merge branch 'next' into kh-add-new-rule
khiga8 Jan 7, 2025
52aa2c8
Fix test
khiga8 Jan 7, 2025
163c50b
Update schema
khiga8 Jan 7, 2025
c235d1f
Fix schema
khiga8 Jan 7, 2025
d090cf1
Update tags
khiga8 Jan 7, 2025
098c40c
undo manual update
khiga8 Jan 8, 2025
02d1166
Add range info
khiga8 Jan 8, 2025
5fd02ee
Address more review comments:
khiga8 Jan 10, 2025
07ef5c0
Address more review comments:
khiga8 Jan 10, 2025
12fc27a
Address more comments
khiga8 Jan 10, 2025
3e8268a
Address more comments
khiga8 Jan 10, 2025
954b98c
Make examples more interesting
khiga8 Jan 10, 2025
1c04710
update config schema
khiga8 Jan 10, 2025
c49f68c
Run npm ci
khiga8 Jan 10, 2025
e2413d4
Surface error for multi-line links
khiga8 Jan 18, 2025
3da8f5c
update snapshots
khiga8 Jan 18, 2025
6237579
fix bracket
khiga8 Jan 18, 2025
1db59f5
Merge branch 'next' into kh-add-new-rule
khiga8 Jan 18, 2025
c62e329
Update snapshots
khiga8 Jan 18, 2025
d062477
Run script again after pulling in latest
khiga8 Jan 18, 2025
d130017
Update snapshots on test repos
khiga8 Jan 18, 2025
8f3d764
remove whitespace
khiga8 Jan 18, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ playground for learning and exploring.
- **[MD055](doc/md055.md)** *table-pipe-style* - Table pipe style
- **[MD056](doc/md056.md)** *table-column-count* - Table column count
- **[MD058](doc/md058.md)** *blanks-around-tables* - Tables should be surrounded by blank lines
- **[MD059](doc/md059.md)** *descriptive-link-text* - Link text should be descriptive

<!-- markdownlint-restore -->

Expand All @@ -165,7 +166,7 @@ To implement your own rules, refer to [CustomRules.md](doc/CustomRules.md).
Tags group related rules and can be used to enable/disable multiple
rules at once.

- **`accessibility`** - `MD045`
- **`accessibility`** - `MD045`, `MD059`
- **`atx`** - `MD018`, `MD019`
- **`atx_closed`** - `MD020`, `MD021`
- **`blank_lines`** - `MD012`, `MD022`, `MD031`, `MD032`, `MD047`
Expand All @@ -183,7 +184,7 @@ rules at once.
- **`language`** - `MD040`
- **`line_length`** - `MD013`
- **`links`** - `MD011`, `MD034`, `MD039`, `MD042`, `MD051`, `MD052`, `MD053`,
`MD054`
`MD054`, `MD059`
- **`ol`** - `MD029`, `MD030`, `MD032`
- **`spaces`** - `MD018`, `MD019`, `MD020`, `MD021`, `MD023`
- **`spelling`** - `MD044`
Expand Down
15 changes: 15 additions & 0 deletions doc-build/md059.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
This rule is triggered when a link is set with non-descriptive text like
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
"Click here", "here", or "learn more", giving it a generic accessible name.

Rationale: Screen reader users may navigate through a list of links
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
to quickly find content on a page. When the link name is something ambiguous
like "Learn more", there isn't sufficient context to help the user determine
whether to follow the link.

Link names should be descriptive and describe the purpose of the link.
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved

To override the default list and configure your own list of banned accessible
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
names, set `banned_link_texts_override` in the config.

Note: At this time, this rule only checks against inline Markdown links given
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
the complexities of determining the accessible name of an HTML anchor tag.
28 changes: 28 additions & 0 deletions doc/Rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -2526,6 +2526,34 @@ Some text
Rationale: In addition to aesthetic reasons, some parsers will incorrectly parse
tables that don't have blank lines before and after them.

<a name="md059"></a>

## `MD059` - Link text should be descriptive

Tags: `accessibility`, `links`

Aliases: `descriptive-link-text`

Parameters:

- `banned_link_texts`: List of banned link texts (`string[]`, default `[]`)

This rule is triggered when a link is set with non-descriptive text like
"Click here", "here", or "learn more", giving it a generic accessible name.

Rationale: Screen reader users may navigate through a list of links
to quickly find content on a page. When the link name is something ambiguous
like "Learn more", there isn't sufficient context to help the user determine
whether to follow the link.

Link names should be descriptive and describe the purpose of the link.

To override the default list and configure your own list of banned accessible
names, set `banned_link_texts_override` in the config.

Note: At this time, this rule only checks against inline Markdown links given
the complexities of determining the accessible name of an HTML anchor tag.

<!-- markdownlint-configure-file {
"no-inline-html": {
"allowed_elements": [
Expand Down
25 changes: 25 additions & 0 deletions doc/md059.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# `MD059` - Link text should be descriptive

Tags: `accessibility`, `links`

Aliases: `descriptive-link-text`

Parameters:

- `banned_link_texts`: List of banned link texts (`string[]`, default `[]`)

This rule is triggered when a link is set with non-descriptive text like
"Click here", "here", or "learn more", giving it a generic accessible name.

Rationale: Screen reader users may navigate through a list of links
to quickly find content on a page. When the link name is something ambiguous
like "Learn more", there isn't sufficient context to help the user determine
whether to follow the link.

Link names should be descriptive and describe the purpose of the link.

To override the default list and configure your own list of banned accessible
names, set `banned_link_texts_override` in the config.

Note: At this time, this rule only checks against inline Markdown links given
the complexities of determining the accessible name of an HTML anchor tag.
21 changes: 3 additions & 18 deletions lib/md039.mjs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// @ts-check

import { addErrorContext } from "../helpers/helpers.cjs";
import { getReferenceLinkImageData, filterByTypesCached } from "./cache.mjs";
import { filterByTypesCached } from "./cache.mjs";

/**
* Adds an error for a label space issue.
Expand Down Expand Up @@ -35,40 +35,25 @@ function addLabelSpaceError(onError, label, labelText, isStart) {
);
}

/**
* Determines if a link is a valid link (and not a fake shortcut link due to parser tricks).
*
* @param {import("markdownlint").MicromarkToken} label Label token.
* @param {import("markdownlint").MicromarkToken} labelText LabelText token.
* @param {Map<string, any>} definitions Map of link definitions.
* @returns {boolean} True iff the link is valid.
*/
function validLink(label, labelText, definitions) {
return (label.parent?.children.length !== 1) || definitions.has(labelText.text.trim());
}

/** @type {import("markdownlint").Rule} */
export default {
"names": [ "MD039", "no-space-in-links" ],
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
"description": "Spaces inside link text",
"tags": [ "whitespace", "links" ],
"parser": "micromark",
"function": function MD039(params, onError) {
const { definitions } = getReferenceLinkImageData();
const labels = filterByTypesCached([ "label" ])
.filter((label) => label.parent?.type === "link");
for (const label of labels) {
const labelTexts = label.children.filter((child) => child.type === "labelText");
for (const labelText of labelTexts) {
if (
(labelText.text.trimStart().length !== labelText.text.length) &&
validLink(label, labelText, definitions)
(labelText.text.trimStart().length !== labelText.text.length)
) {
addLabelSpaceError(onError, label, labelText, true);
}
if (
(labelText.text.trimEnd().length !== labelText.text.length) &&
validLink(label, labelText, definitions)
(labelText.text.trimEnd().length !== labelText.text.length)
) {
addLabelSpaceError(onError, label, labelText, false);
}
Expand Down
52 changes: 52 additions & 0 deletions lib/md059.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// @ts-check

import { addErrorContext } from "../helpers/helpers.cjs";
import { filterByTypesCached } from "./cache.mjs";

const defaultBannedNames = [
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
"click here",
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
"here",
"learn more",
"link",
"more",
"read more"
];

/**
* Downcases a string and strips extra white spaces and punctuations.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Downcases" -> "Normalizes"
"and strips" -> "by removing"

*
* @param {string} text String to transform.
* @returns {string} Stripped and downcased string.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

*/
function normalizeText(text) {
return text
.toLowerCase()
.replace(/\W+/g, " ")
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
.replace(/\s+/g, " ")
.trim();
}

/** @type {import("markdownlint").Rule} */
export default {
"names": [ "MD059", "descriptive-link-text" ],
"description": "Link text should be descriptive",
"tags": [ "links", "accessibility" ],
"parser": "micromark",
"function": function MD059(params, onError) {
// Allow consumer to define their own banned names list.
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
const bannedNames = new Set(params.config.banned_link_texts_override || defaultBannedNames);

const labels = filterByTypesCached([ "label" ])
.filter((label) => label.parent?.type === "link");

for (const label of labels) {
const labelTexts = label.children.filter((child) => child.type === "labelText");
for (const labelText of labelTexts) {
const { text } = label;
if (bannedNames.has(normalizeText(text))) {
addErrorContext(onError, labelText.startLine, text);
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}
};
4 changes: 3 additions & 1 deletion lib/rules.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import md054 from "./md054.mjs";
import md055 from "./md055.mjs";
import md056 from "./md056.mjs";
import md058 from "./md058.mjs";
import md059 from "./md059.mjs";

const rules = [
md001,
Expand Down Expand Up @@ -108,7 +109,8 @@ const rules = [
md055,
md056,
// md057: See https://github.com/markdownlint/markdownlint
md058
md058,
md059
];
for (const rule of rules) {
const name = rule.names[0].toLowerCase();
Expand Down
36 changes: 36 additions & 0 deletions schema/markdownlint-config-schema-strict.json
Original file line number Diff line number Diff line change
Expand Up @@ -1716,6 +1716,42 @@
"type": "boolean",
"default": true
},
"MD059": {
"description": "MD059/descriptive-link-text : Link text should be descriptive : https://github.com/DavidAnson/markdownlint/blob/v0.37.3/doc/md059.md",
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
"type": [
"boolean",
"object"
],
"properties": {
"banned_link_texts": {
"description": "List of banned link texts",
"type": "array",
"items": {
"type": "string"
},
"default": []
}
},
"default": true
},
"descriptive-link-text": {
"description": "MD059/descriptive-link-text : Link text should be descriptive : https://github.com/DavidAnson/markdownlint/blob/v0.37.3/doc/md059.md",
"type": [
"boolean",
"object"
],
"properties": {
"banned_link_texts": {
"description": "List of banned link texts",
"type": "array",
"items": {
"type": "string"
},
"default": []
}
},
"default": true
},
"headings": {
"description": "headings : MD001, MD003, MD018, MD019, MD020, MD021, MD022, MD023, MD024, MD025, MD026, MD036, MD041, MD043",
"type": "boolean",
Expand Down
30 changes: 30 additions & 0 deletions schema/markdownlint-config-schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1716,6 +1716,36 @@
"type": "boolean",
"default": true
},
"MD059": {
"description": "MD059/descriptive-link-text : Link text should be descriptive : https://github.com/DavidAnson/markdownlint/blob/v0.37.3/doc/md059.md",
"type": "boolean",
"properties": {
"banned_link_texts": {
"description": "List of banned link texts",
"type": "array",
"items": {
"type": "string"
},
"default": []
}
},
"default": true
},
"descriptive-link-text": {
"description": "MD059/descriptive-link-text : Link text should be descriptive : https://github.com/DavidAnson/markdownlint/blob/v0.37.3/doc/md059.md",
"type": "boolean",
"properties": {
"banned_link_texts": {
"description": "List of banned link texts",
"type": "array",
"items": {
"type": "string"
},
"default": []
}
},
"default": true
},
"headings": {
"description": "headings : MD001, MD003, MD018, MD019, MD020, MD021, MD022, MD023, MD024, MD025, MD026, MD036, MD041, MD043",
"type": "boolean",
Expand Down
2 changes: 1 addition & 1 deletion test/bare-urls.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Visit https://example.com, then refresh. {MD034}

The site (https://example.com) is down. {MD034}

<!-- markdownlint-disable line-length no-inline-html -->
<!-- markdownlint-disable line-length no-inline-html descriptive-link-text -->
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved

Some documents use <a href="https://example.com">to link</a>.

Expand Down
17 changes: 17 additions & 0 deletions test/descriptive-link-text-override-config.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Descriptive link text override config
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved

[Go here](https://example.com/javascript/about) {MD059}

[Learn more](https://example.com/javascript/about).

[Click here](https://example.com/javascript/about).

To learn more, go [here!](https://example.com/javascript/about).

To learn more, go to this [link!](https://example.com/javascript/about).

<!-- markdownlint-configure-file {
"descriptive-link-text": {
"banned_link_texts_override": ["go here"]
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved
}
} -->
23 changes: 23 additions & 0 deletions test/descriptive-link-text.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Descriptive link text
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved

[Learn about Javascript](https://example.com/javascript/about)
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved

[Read about Javascript](https://example.com/javascript/about)

Learn about [JavaScript](https://example.com/javascript/about).
DavidAnson marked this conversation as resolved.
Show resolved Hide resolved

[Learn more](https://example.com/javascript/about). {MD059}

[Click here](https://example.com/javascript/about). {MD059}

To learn more, go [here!](https://example.com/javascript/about). {MD059}

To learn more, [click here!!!!](https://example.com/javascript/about). {MD059}

[click-here!!!!](https://example.com/javascript/about). {MD059}

Go to this [link]((https://example.com/javascript/about)). {MD059}

[link][1] {MD059}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DavidAnson Is this what you meant by reference link?
Do you have an example of a "shortcut link"?

related: #1459 (comment)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yours is a reference link, but it looks more like a footnote. Let's use something more typical as shown in the examples I link.


[1]: https://example.com/javascript/about
2 changes: 1 addition & 1 deletion test/fixing-with-front-matter.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ ignore: this
# Fixing with Front Matter {MD022}
Text text text {MD009}

Text [ link ](url) text {MD039}
Text [ link ](url) text {MD039} {MD059}
## Nested Heading {MD022}

Text {MD047}
1 change: 1 addition & 0 deletions test/link-style-no-url-inline-not-possible.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ Text [email] text
[email]: user@example.com

<!-- markdownlint-configure-file {
"descriptive-link-text": false,
"link-image-style": {
"autolink": false,
"url_inline": false
Expand Down
1 change: 1 addition & 0 deletions test/link-style-no-url-inline-possible.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ Text [email] text
[email]: user@example.com

<!-- markdownlint-configure-file {
"descriptive-link-text": false,
"link-image-style": {
"url_inline": false
}
Expand Down
3 changes: 2 additions & 1 deletion test/links-alternate.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,6 @@ Text [ link ][reference] text. {MD039}

<!-- markdownlint-configure-file {
"line-length": false,
"code-block-style": false
"code-block-style": false,
"descriptive-link-text": false
} -->
1 change: 1 addition & 0 deletions test/long-lines-long-reference-definitions-stern.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
[long-reference-definition]: https://example.com/long/long/long/long/long/long/long/long/long/long/long/long/long

<!-- markdownlint-configure-file {
"descriptive-link-text": false,
"line-length": {
"stern": true
}
Expand Down
Loading