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

Closed
mdjermanovic opened this issue Jun 24, 2021 · 9 comments · Fixed by #14769 or #17715
Closed

Remove SourceCode#getComments() #14744

mdjermanovic opened this issue Jun 24, 2021 · 9 comments · Fixed by #14769 or #17715
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features

Comments

@mdjermanovic
Copy link
Member

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 added core Relates to ESLint's core APIs and features breaking This change is backwards-incompatible labels Jun 24, 2021
@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 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

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

@nzakas
Copy link
Member

nzakas commented Jun 25, 2021

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

@mdjermanovic
Copy link
Member Author

Prepared a quick fix: #14748

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

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 commented Jun 29, 2021

Throwing an error in RuleTester sounds good.

@mdjermanovic
Copy link
Member Author

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 Aug 5, 2021 that will close this issue
1 task
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

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 commented Aug 6, 2021

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

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Nov 4, 2023
mdjermanovic added a commit that referenced this issue Nov 4, 2023
Public Roadmap automation moved this from Planned to Complete Dec 20, 2023
mdjermanovic added a commit that referenced this issue Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features
Projects
Public Roadmap
  
Complete
Archived in project
Status: Done
Triage
Blocked
v9.0.0
Need Discussion
2 participants