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

Introduce option to prevent underlining multiple lines #1528

Merged
merged 7 commits into from
Nov 23, 2022

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Sep 26, 2022

I rebased #463 in hopes that you'll reconsider merging this useful feature.

Your main concern seems to be consistency, but that's not a concern shared by ESLint maintainers:

In #8004, it was decided that ESLint should report large ranges if a node with a large range is reported by a rule, and allow integrations to display a smaller report if desired. The reasoning is that different integrations will have different needs with regard to report ranges (for example, an integration that posts comments on GitHub might want to know the entire reported range, whereas an editor integration might only want to highlight a subset of a reported range).
Source

vscode-eslint-singleLineUnderline.mov

@dbaeumer
Copy link
Member

I think the video is a good example why underlining the first line is not a good idea. The error is about the fact that role="button" implies some other property. Only underlining <div might not be very helpful.

@aleclarson
Copy link
Contributor Author

@dbaeumer That's a tradeoff that the user should be able to make on their own

server/src/eslint.ts Outdated Show resolved Hide resolved
$shared/settings.ts Outdated Show resolved Hide resolved
@@ -540,6 +540,8 @@ export namespace RuleSeverities {
}
}

const documentsByProblem = new WeakMap<ESLintProblem, TextDocument>();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this. Wouldn't it be good enough to pass the document to Diagnostics.create

line: startLine,
character: 0,
},
end: {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be end: { line: startLine, character: uinteger.MAX_VALUE }

},
end: {
line: startLine,
character: Number.MAX_SAFE_INTEGER,
Copy link
Member

Choose a reason for hiding this comment

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

This should be uinteger.MAX_VALUE do to the restrictions we have in LSP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, didn't know that was a thing :)

Copy link
Member

@dbaeumer dbaeumer left a comment

Choose a reason for hiding this comment

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

See comment about uinteger.MAX_VALUE

@dbaeumer
Copy link
Member

The changes look good to me.

@aleclarson you need to accept the CLA (see #1528 (comment)). I will not be able to merge this without you accepting it.

@aleclarson
Copy link
Contributor Author

@microsoft-github-policy-service agree

@dbaeumer dbaeumer merged commit 578cd50 into microsoft:main Nov 23, 2022
@dbaeumer dbaeumer added this to the 2.4.0 milestone Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants