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

[khiga8]

Closes: #681

This PR introduces a new lint rule to discourage generic, unhelpful link names. More context in linked issue!

While this rule has a default set of "banned names", consumers have the option to override the default and set their own banned words through the link_texts config if they'd like.

[khiga8]

I'm not sure how to interpret this failure:

 test › markdownlint-test › validateJsonUsingConfigSchemaStrict

  Rejected promise returned by test. Reason:

  SyntaxError {
    message: 'Unexpected token } in JSON at position 42',
  }

I'd appreciate pointers!

[DavidAnson]

Great! I'll make a first pass review later tonight. Regarding the test failure you ask about, I think it is due to the trailing commas added to the new configuration comment at the bottom of test/long-lines-long-reference-definitions.md. JSON does not allow trailing commas.

In general, I think this would be the first built-in rule which only works in a single language (English) in its default configuration. I'm not sure how I feel about that... Certainly, all the documentation is in English because that's all I know and it's fairly common as a default language in the industry for documentation and code. However, to have the linting tool itself start to "favor" one language over another feels a little weird?

I'm open to counter-arguments or examples of another built-in rule that behaves like this. At the same time, having this rule default to an empty list would make it less useful. (Though there is precedent for that, see MD044/proper-names.) I do not think I want to start maintaining multiple translations for the phrase list because there are a lot of languages out there and I can only validate English for correctness.

[DavidAnson]

I was curious how a similar-ish project handled this situation: get-alex/alex#202

Ugh, that is not really a road I went to start down. My current thinking is to do something similar to MD044/proper-names and default this rule to an empty list.

I'm still open to feedback, though!

[DavidAnson]

Okay, it may look like a lot, but most of the changes are pretty small. :) Thanks for starting this out - let's continue the conversation around naming and default value in the body of the PR.

[khiga8]

Hey @DavidAnson, thank you for your speedy and thorough review!

I like your suggestion of renaming this to descriptive-link-text, so I've made corresponding updates in this PR.

Certainly, all the documentation is in English because that's all I know and it's fairly common as a default language in the industry for documentation and code. However, to have the linting tool itself start to "favor" one language over another feels a little weird?

My current thinking is to do something similar to MD044/proper-names and default this rule to an empty list.

For sure, these are valid concerns!

My personal preference is for setting a default list for the English language (since we have a general sense of common, "non-descriptive names" for the following reasons:

• If consumers of this project want to expand on the default words, disagree with the defaults, or add support for additional languages, they have the option to do so through the config.
• Setting some kind of default would make this rule useful for more projects immediately and help spread awareness around best a11y practices versus having to discover this through the docs.

I guess there's also a question around whether this rule should be on/off by default (not sure if you have a standard procedure for new rules?) If this rule is on by default with a list of defaults, some projects will encounter a lot of failure when bumping up the version. But then again, maybe that's okay because if a project doesn't feel ready to enforce the rule, they can turn it off.

Let me know what you think!

[khiga8]

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

related: #1459 (comment)

[DavidAnson]

Examples here: https://github.com/DavidAnson/markdownlint/blob/main/doc/md054.md

[DavidAnson]

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.

[DavidAnson]

All rules are enabled by default and I'd like to keep it that way. You make a good point about this being more discoverable if there are default values. Then I'd want to document that there will ONLY be English defaults so people don't PR Klingon or something. I don't mind it breaking for existing sites, but I want to be sure the default list is super obviously bad. Like "here" bad. People can disagree, but nobody should argue that "here" is a good link text. :)

[DavidAnson]

Looking good, thanks for making all those edits!! After going through these new comments, please update schema/build-config-schema.mjs which will automatically update some of the other files like the schema, etc., once you run npm run ci. I see test failures, but assume it's mostly stuff like this. You may get failures in the TestRepos workflow - use npm run update-snapshots and npm run clone-test-repos then npm run update-snapshots-test-repos to automatically update all the snapshot files. We can have a look at what impact this new rule has on the test repos based on that.

[DavidAnson]

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.

[khiga8]

 -       errorContext: `[click␍␊
 +       errorContext: `[click␊
         here]`,

I can't figure out why Windows behaves differently and returns a different text for the link test case with a line break. Curious if you might have any ideas @DavidAnson.

[DavidAnson]

This looks like a \r\n vs. \n line ending difference. If you're getting this error, it's reporting an error that spans multiple lines. I left a comment about preventing that by checking startLine and endLine of the span. (See my comment on the main chat before spending time on this.)

[khiga8]

Hey @DavidAnson! I believe I've addressed all your comments now. I have one remaining issue I'd appreciate your eyes on - #1459 (comment).

[DavidAnson]

Thank you! I replied to that open issue above. (FYI, I may not get back to this for a couple of days.)

At this stage in the process, I'll make another/final pass over the entire change. It's likely I'll have more nitpicks. Are you okay with me applying them to your PR here? What I typically do is squash the author's comments into one (attributed to then) with a following commit by me titled something like "final tweaks for previous commit". Unless you enjoy my nitpicking, this is probably the most efficient option. :)

One thing I'll do is review every new violation in test-repos. Skimming that now, it looks like the vast majority are "here" and "link" which is expected (plus a couple of "more"s). However, there are about 20 like this in mdn which deserve better understanding. If you beat me to that, please reply here with your thoughts:

test-repos/mdn-content/files/en-us/web/html/attributes/rel/preload/index.md: 105: MD059/descriptive-link-text Link text should be descriptive [Context: "[\`<link>\`]"]