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

Remove SourceCode#getComments() #14744

Open
mdjermanovic opened this issue Jun 24, 2021 · 9 comments
Open

Remove SourceCode#getComments() #14744

mdjermanovic opened this issue Jun 24, 2021 · 9 comments

Comments

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jun 24, 2021

The version of ESLint you are using.

ESLint 7.29.0 with Espree 8.0.0-beta.1

The problem you want to solve.

This test is failing:

describe("getComments()", () => {
const config = { rules: { checker: "error" }, parserOptions: { ecmaVersion: 6, sourceType: "module" } };
let unusedAssertionFuncs;
beforeEach(() => {
unusedAssertionFuncs = new Set();
});
/**
* Check comment count
* @param {int} leading Leading comment count
* @param {int} trailing Trailing comment count
* @returns {Function} function to execute
* @private
*/
function assertCommentCount(leading, trailing) {
/**
* Asserts the comment count for a node
* @param {ASTNode} node the node being traversed
* @returns {void}
*/
function assertionFunc(node) {
unusedAssertionFuncs.delete(assertionFunc);
const sourceCode = linter.getSourceCode();
const comments = sourceCode.getComments(node);
assert.strictEqual(comments.leading.length, leading);
assert.strictEqual(comments.trailing.length, trailing);
}
unusedAssertionFuncs.add(assertionFunc);
return assertionFunc;
}
afterEach(() => {
assert.strictEqual(
unusedAssertionFuncs.size,
0,
"An assertion function was created with assertCommentCount, but the function was not called."
);
});
it("should return comments around nodes", () => {
const code = [
"// Leading comment for VariableDeclaration",
"var a = 42;",
"/* Trailing comment for VariableDeclaration */"
].join("\n");
linter.defineRule("checker", () => ({
Program: assertCommentCount(0, 0),
VariableDeclaration: assertCommentCount(1, 1),
VariableDeclarator: assertCommentCount(0, 0),
Identifier: assertCommentCount(0, 0),
Literal: assertCommentCount(0, 0)
}));
assert.isEmpty(linter.verify(code, config));
});

because sourceCode.getComments(node) uses start and end properties of nodes and tokens:

while (currentToken && isCommentToken(currentToken)) {
if (node.parent && (currentToken.start < node.parent.start)) {
break;
}
comments.leading.push(currentToken);
currentToken = this.getTokenBefore(currentToken, { includeComments: true });
}

and after the change in eslint/espree#461, Program.start is no longer 0 but body[0].start:

https://github.com/eslint/espree/blob/e08c9d78745fcee03136b57f46d09e11cad70861/lib/espree.js#L171

Your take on the correct solution to problem.

Remove SourceCode#getComments()?

It was deprecated in ESLint v4.0.0 (735d02d):

Please note that the following methods have been deprecated and will be removed in a future version of ESLint:

  • getComments() - replaced by getCommentsBefore(), getCommentsAfter(), and getCommentsInside()

In ESLint v7.0.0, RuleTester was updated to throw on any access to start and end properties. Consequently, rules that use sourceCode.getComments() should have already noticed errors (e.g., #13293) and switched to other methods.

Are you willing to submit a pull request to implement this change?

Yes

@mdjermanovic mdjermanovic self-assigned this Jun 24, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Jun 24, 2021
@mdjermanovic mdjermanovic moved this from Needs Triage to Feedback Needed in Triage Jun 24, 2021
@nzakas
Copy link
Member

@nzakas nzakas commented Jun 25, 2021

I'm not opposed to removing it but since we already locked down and announced the changes coming in v8.0.0, it seems like we should wait until v9.0.0 and maybe just add a deprecation warning in v8.0.0?

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Jun 25, 2021

Then we probably should fix it, since it will be broken with Espree 8.

@nzakas
Copy link
Member

@nzakas nzakas commented Jun 25, 2021

Agreed, sorry, that's what I meant to imply.

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Jun 26, 2021

Prepared a quick fix: #14748

@mdjermanovic mdjermanovic added this to Need Discussion in v9.0.0 Jun 26, 2021
@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Jun 26, 2021

I added this to v9.0.0 project.

maybe just add a deprecation warning in v8.0.0

That would be throwing an error in RuleTester? It already throws on any access to start/end properties, so that might be good enough, though not all code paths in SourceCode#getComments() use start/end .

@nzakas
Copy link
Member

@nzakas nzakas commented Jun 29, 2021

Throwing an error in RuleTester sounds good.

mdjermanovic added a commit that referenced this issue Jul 2, 2021
…) (#14748)
mdjermanovic added a commit that referenced this issue Jul 5, 2021
@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Jul 6, 2021

Prepared #14769 to throw an error in RuleTester if SourceCode#getComments() was called, starting from ESLint v8.0.0.

The remaining task is to completely remove SourceCode#getComments() in ESLint v9.0.0.

@nzakas nzakas moved this from Feedback Needed to Blocked in Triage Jul 16, 2021
@nzakas nzakas linked a pull request that will close this issue Aug 5, 2021
1 task
@nzakas nzakas closed this in #14769 Aug 5, 2021
Triage automation moved this from Blocked to Complete Aug 5, 2021
nzakas added a commit that referenced this issue Aug 5, 2021
… (#14769)

* Breaking: disallow SourceCode#getComments() in RuleTester (refs #14744)

* Update lib/rule-tester/rule-tester.js

Co-authored-by: Nicholas C. Zakas <nicholas@nczconsulting.com>

Co-authored-by: Nicholas C. Zakas <nicholas@nczconsulting.com>
@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Aug 6, 2021

Reopening since it isn't completed (there's still a task to remove the method in v9.0.0).

@mdjermanovic mdjermanovic reopened this Aug 6, 2021
Triage automation moved this from Complete to Evaluating Aug 6, 2021
@nzakas
Copy link
Member

@nzakas nzakas commented Aug 6, 2021

Weird. No idea what could have caused it to close. Nice catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Public Roadmap
  
Planned
Triage
Blocked
v9.0.0
Need Discussion
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants