Handle preexisting disable rule comments by asingh04 · Pull Request #1261 · microsoft/vscode-eslint (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation16 Commits16 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Referencing the issue #1248

…preexisting-disable-rule-comment

…-preexisting-disable-rule-comment

All CLA requirements met.

…-preexisting-disable-rule-comment

Will have a look this month.

…-preexisting-disable-rule-comment

Choose a reason for hiding this comment

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

Can you please exclude the package-lock changes from the PR. Otherwise very hard to merge.

if ( editInfo.line - 1 > 0) {

// check previous line if there is a eslint-disable-next-line comment already present
const prevLine = textDocument?.getText(Range.create(Position.create(editInfo.line - 2, 0), Position.create(editInfo.line - 2, Number.MAX_VALUE)));

Choose a reason for hiding this comment

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

We should use uinteger.MAX_VALUE which is defined in the latest LSP protocol and used in ESLint.

Choose a reason for hiding this comment

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

changed to uinteger.MAX_VALUE

return TextEdit.insert(Position.create(editInfo.line - 1, 0), `${indentationText}// eslint-disable-next-line editInfo.ruleId{editInfo.ruleId}editInfo.ruleId{EOL}`);
}

function createDisableSameLineTextEdit(editInfo: Problem): TextEdit {
// Todo@dbaeumer Use uinteger.MAX_VALUE instead.
return TextEdit.insert(Position.create(editInfo.line - 1, 2147483647), ` // eslint-disable-line ${editInfo.ruleId}`);
const currentLine = textDocument?.getText(Range.create(Position.create(editInfo.line - 1, 0), Position.create(editInfo.line -1, Number.MAX_VALUE)));

Choose a reason for hiding this comment

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

Same here

Choose a reason for hiding this comment

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

changed to uinteger.MAX_VALUE

@@ -1933,8 +1947,7 @@ messageQueue.registerRequest(CodeActionRequest.type, (params) => {
if (settings.codeAction.disableRuleComment.location === 'sameLine') {
workspaceChange.getTextEditChange({ uri, version: documentVersion }).add(createDisableSameLineTextEdit(editInfo));
} else {
// Todo@dbaeumer Use uinteger.MAX_VALUE instead.
const lineText = textDocument.getText(Range.create(Position.create(editInfo.line - 1, 0), Position.create(editInfo.line - 1, 2147483647)));
const lineText = textDocument.getText(Range.create(Position.create(editInfo.line - 1, 0), Position.create(editInfo.line - 1, Number.MAX_VALUE)));

Choose a reason for hiding this comment

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

Same here

Choose a reason for hiding this comment

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

changed to uinteger.MAX_VALUE

…-preexisting-disable-rule-comment

Can you please exclude the package-lock changes from the PR. Otherwise very hard to merge.

Reverted the change in all package-lock.json files

Hi @dbaeumer, the changes you had recommended are done. Please go through this PR.

…-preexisting-disable-rule-comment

@asingh04 sorry slipped through the cracks. When reviewing I noticed that the code has a lot of ? and ! which shouldn't be there. I fixed this. Could you merge again and use the textDocument passed in as a param. Makes the code clearer and easier to understand.

Thanks

…-preexisting-disable-rule-comment

@asingh04 sorry slipped through the cracks. When reviewing I noticed that the code has a lot of ? and ! which shouldn't be there. I fixed this. Could you merge again and use the textDocument passed in as a param. Makes the code clearer and easier to understand.

Thanks

Oh yes, it is my mistake for not noting that textDocument is used with optional chaining in the function, and straight ahead went to use ! for reading languageId. I have made the changes according to you recommendation.

Thanks for doing the changes.

2 participants