Skip to content

fix: no-*-definitions rules show original label and identifier #515

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.

Sign up for GitHub

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

Jump to bottom
Merged
mdjermanovic merged 3 commits into from
Sep 13, 2025
Merged

fix: no-*-definitions rules show original label and identifier #515

mdjermanovic merged 3 commits into from
Sep 13, 2025

Conversation

@jaymarvelz
Copy link
Contributor

@jaymarvelz jaymarvelz commented Aug 31, 2025

Prerequisites checklist

What is the purpose of this pull request?

The purpose is to improve the clarity and usability of linting feedback by ensuring messages reflect the exact labels written in Markdown, helping users quickly identify and fix issues without confusion.

What changes did you make? (Give an overview)

Updated the no-*-definitions rules so that reported messages display the original label as written in the Markdown source, instead of the normalized identifier. This makes error messages clearer and consistent with user expectations.

Related Issues

Closes #514

Is there anything you'd like reviewers to focus on?

@eslint-github-bot eslint-github-bot bot mentioned this pull request Aug 31, 2025
Closed
1 task
@eslintbot eslintbot added this to Triage Aug 31, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Aug 31, 2025
mdjermanovic
mdjermanovic reviewed Sep 1, 2025
View reviewed changes
@mdjermanovic mdjermanovic moved this from Needs Triage to Implementing in Triage Sep 1, 2025
lumirlumir
lumirlumir reviewed Sep 1, 2025
View reviewed changes
Copy link
Member

@lumirlumir lumirlumir left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm +1 with exposing the label in the message, but I'm not sure removing the identifier entirely from message is the best approach.

Could we expose both the identifier and the label in the message? Here are some reasons:

Identifiers may be normalized in non-English languages (and, for English, all letters are converted to lowercase). For example:

markdown/tests/rules/no-duplicate-definitions.test.js

Lines 162 to 173 in 4d4f124

code: `
[Grüsse]: https://example.com/
[Grüsse]: https://example.com/
`,
options: [{ allowDefinitions: ["GRÜẞE"] }],
},
{
code: `
[^Grüsse]: Grüsse
[^Grüsse]: Grüsse
`,
options: [{ allowFootnoteDefinitions: ["GRÜẞE"] }],

If we write Grüsse and GRÜẞE, the rule reports them as the same definition identifier.

That could be confusing if only labels are shown. For example, a user might intentionally create one definition as Grüsse and another as GRÜẞE (Actually, there are many cases like this), yet the rule flags a conflict because the identifier is normalized. This may confuse users who aren't aware that definition identifiers are normalized.

So, If others agree, I’d like to suggest using a message format like the following:

- Unexpected duplicate definition `{{ identifier }}` found.
+ Unexpected duplicate definition `{{ identifier }}` (label: `{{ label }}`) found.

(The message format may vary if other suggestions are available.)

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 2, 2025

So, If others agree, I’d like to suggest using a message format like the following:

- Unexpected duplicate definition `{{ identifier }}` found.
+ Unexpected duplicate definition `{{ identifier }}` (label: `{{ label }}`) found.

(The message format may vary if other suggestions are available.)

Sounds good to me to include both identifier and label. Ideally, the message could also include location and label of the first definition (the one of which this is a duplicate of).

@jaymarvelz
Copy link
Contributor Author

jaymarvelz commented Sep 3, 2025

Ideally, the message could also include location and label of the first definition (the one of which this is a duplicate of).

How about this phrasing?

"Unexpected duplicate definition `{{ identifier }}` (label: `{{ label }}`) found. First defined at line {{ firstLine }} (label: `{{ firstLabel }}`)."

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 3, 2025

Ideally, the message could also include location and label of the first definition (the one of which this is a duplicate of).

How about this phrasing?

"Unexpected duplicate definition `{{ identifier }}` (label: `{{ label }}`) found. First defined at line {{ firstLine }} (label: `{{ firstLabel }}`)."

Looks good to me 👍

@mdjermanovic mdjermanovic changed the title fix: no-*-definitions rules show original label instead of identifier fix: no-*-definitions rules show original label and identifier Sep 6, 2025
mdjermanovic
mdjermanovic previously approved these changes Sep 6, 2025
View reviewed changes
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Leaving open for @lumirlumir to verify.

@mdjermanovic mdjermanovic moved this from Implementing to Second Review Needed in Triage Sep 6, 2025
lumirlumir
lumirlumir requested changes Sep 7, 2025
View reviewed changes
lumirlumir
lumirlumir approved these changes Sep 13, 2025
View reviewed changes
Copy link
Member

@lumirlumir lumirlumir left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks.

Would like @mdjermanovic to verify the changes before merging.

mdjermanovic
mdjermanovic approved these changes Sep 13, 2025
View reviewed changes
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit f80a9e1 into eslint:main Sep 13, 2025
23 checks passed
@github-project-automation github-project-automation bot moved this from Second Review Needed to Complete in Triage Sep 13, 2025
@github-actions github-actions bot mentioned this pull request Sep 12, 2025
Merged
@fossabot fossabot bot mentioned this pull request Oct 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mdjermanovic mdjermanovic mdjermanovic approved these changes

@lumirlumir lumirlumir lumirlumir approved these changes

Assignees

No one assigned

Labels

accepted bug contributor pool

Projects

Triage
Status: Complete

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: no-*-definitions rules show normalized identifier in messages

3 participants

@jaymarvelz @mdjermanovic @lumirlumir