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

Handle preexisting disable rule comments #1261

Merged

Conversation

asingh04
Copy link
Contributor

@asingh04 asingh04 commented May 14, 2021

Referencing the issue #1248

@ghost
Copy link

ghost commented May 14, 2021

CLA assistant check
All CLA requirements met.

@asingh04
Copy link
Contributor Author

asingh04 commented Jun 8, 2021

@dbaeumer can you have a look into it?

@dbaeumer
Copy link
Member

dbaeumer commented Jun 9, 2021

Will have a look this month.

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.

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)));
Copy link
Member

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.

Copy link
Contributor Author

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}${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)));
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

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)));
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

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

@asingh04
Copy link
Contributor Author

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

@asingh04 asingh04 requested a review from dbaeumer August 28, 2021 11:45
@asingh04
Copy link
Contributor Author

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

@dbaeumer dbaeumer added this to the 2.2.2 milestone Oct 8, 2021
@asingh04
Copy link
Contributor Author

@dbaeumer resolved the merge conflicts

@dbaeumer
Copy link
Member

@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

@asingh04
Copy link
Contributor Author

@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.

@dbaeumer
Copy link
Member

Thanks for doing the changes.

@dbaeumer dbaeumer merged commit e14b86e into microsoft:main Nov 16, 2021
@dbaeumer dbaeumer modified the milestones: 2.2.next, 2.2.6 Jul 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.

2 participants