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

feat!: Remove deprecated context methods #17698

Merged
merged 12 commits into from Dec 20, 2023
Merged

feat!: Remove deprecated context methods #17698

merged 12 commits into from Dec 20, 2023

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Oct 31, 2023

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Removed the deprecated methods on the context object.

fixes #17520

Is there anything you'd like reviewers to focus on?

@eslint-github-bot eslint-github-bot bot added breaking This change is backwards-incompatible feature This change adds a new feature to ESLint labels Oct 31, 2023
Copy link

netlify bot commented Oct 31, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit ffb5d0a
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/657b2df96955f300081fe380
😎 Deploy Preview https://deploy-preview-17698--docs-eslint.netlify.app/extend/custom-rules
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion labels Nov 4, 2023
{}
)
);
const BASE_TRAVERSAL_CONTEXT = {};
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need BASE_TRAVERSAL_CONTEXT? Looks like we can just remove it now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, right you are!

@mdjermanovic
Copy link
Member

We should also remove deprecation warnings from the RuleTester:

getSource: "getText",
getSourceLines: "getLines",
getAllComments: "getAllComments",
getNodeByRangeIndex: "getNodeByRangeIndex",
// getComments: "getComments", -- already handled by a separate error
getCommentsBefore: "getCommentsBefore",
getCommentsAfter: "getCommentsAfter",
getCommentsInside: "getCommentsInside",
getJSDocComment: "getJSDocComment",
getFirstToken: "getFirstToken",
getFirstTokens: "getFirstTokens",
getLastToken: "getLastToken",
getLastTokens: "getLastTokens",
getTokenAfter: "getTokenAfter",
getTokenBefore: "getTokenBefore",
getTokenByRangeStart: "getTokenByRangeStart",
getTokens: "getTokens",
getTokensAfter: "getTokensAfter",
getTokensBefore: "getTokensBefore",
getTokensBetween: "getTokensBetween",

@nzakas
Copy link
Member Author

nzakas commented Nov 6, 2023

Oops, yes. Removed from the tests but not RuleTester itself. Good catch.

Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Nov 16, 2023
@Rec0iL99 Rec0iL99 removed the Stale label Nov 17, 2023

});

describe("context.getAncestors()", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Tests for context.getAncestors() shouldn't be removed in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like getAncestors(), getScope(), getDeclaredVariables(), and markVariableAsUsed() don't actually have a separate issue tracking their removal, despite us announcing it.

So maybe I should remove those in this PR too?

Copy link
Member

Choose a reason for hiding this comment

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

It's a subtask of #16999 (Phase 8: Removal of old APIs; just it seems that markVariableAsUsed() was accidentally omitted from that list, and note that the list includes removing context.parserServices as well) which is tracked in the v9 project.

Regardless, seems best and easiest for maintaining the alpha release to remove them as well in this PR 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good call, those subtasks are tricky to remember. I've updated the list to reflect reality and will add the rest into this PR.

@nzakas
Copy link
Member Author

nzakas commented Nov 22, 2023

Note: Linting is failed because eslint-plugin-n is still using the old getScope() and getAncestors(), which have been removed. I opened an issue here:
eslint-community/eslint-plugin-n#143

Copy link

github-actions bot commented Dec 2, 2023

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

Copy link

This pull request was auto-closed due to inactivity. While we wish we could keep working on every request, we unfortunately don't have the bandwidth to continue here and need to focus on other things. You can resubmit this pull request if you would like to continue working on it.

@github-actions github-actions bot closed this Dec 10, 2023
@aladdin-add aladdin-add reopened this Dec 11, 2023
@aladdin-add
Copy link
Member

aladdin-add commented Dec 11, 2023

@mdjermanovic
Copy link
Member

I've made a few updates, hopefully it can fix the lint failing:

All merged now. I checked this branch locally and linting works now. There are just several lint errors and a merge conflict in package.json that should be fixed.

@nzakas
Copy link
Member Author

nzakas commented Dec 11, 2023

Fixed merge conflicts and re-pushed the branch.

@aladdin-add
Copy link
Member

seems there are a few linting errors that need to be fixed.

@nzakas
Copy link
Member Author

nzakas commented Dec 12, 2023

Thanks. Fixed!

Co-authored-by: 唯然 <weiran.zsd@outlook.com>
docs/src/extend/custom-rules.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rules.md Outdated Show resolved Hide resolved
docs/src/extend/custom-rules.md Outdated Show resolved Hide resolved
assert.strictEqual(suppressedMessages.length, 0);
});

it("should expose parser services when using parseForESLint() and services are specified", () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep tests with parserServices as they're testing how Linter creates SourceCode objects based on parser features. We could just remove the assert.strictEqual(context.parserServices, context.sourceCode.parserServices) line.

assert.strictEqual(suppressedMessages.length, 0);
});

it("should use the same parserServices if source code object is reused", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Same as the previous comment, I think we should keep this test here.


it("should expose parser services when using parseForESLint() and services are specified", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Same as the previous comment, I think we should keep this test here.


it("should use the same parserServices if source code object is reused", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Same as the previous comment, I think we should keep this test here.

Copy link
Member

Choose a reason for hiding this comment

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

Just a tip if anyone else is going to review this: better use VS Code diff viewer for this file, as this diff here is pretty much unusable and the diff is not as large as it appears to be.

nzakas and others added 5 commits December 14, 2023 10:51
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@nzakas
Copy link
Member Author

nzakas commented Dec 14, 2023

Also updated eslint-plugin-n so linting will no longer fail. 🎉

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!


describe("when calling context.getScope", () => {
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that this set of tests wasn't copied into source-code tests, but we could do that in another PR.

});
});

describe("Variables and references", () => {
Copy link
Member

Choose a reason for hiding this comment

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

This set of tests also doesn't exist in source-code tests, though these look more like tests for eslint-scope than tests for linter.

@mdjermanovic mdjermanovic marked this pull request as ready for review December 20, 2023 13:41
@mdjermanovic mdjermanovic requested a review from a team as a code owner December 20, 2023 13:41
@mdjermanovic mdjermanovic merged commit ae78ff1 into main Dec 20, 2023
22 checks passed
@mdjermanovic mdjermanovic deleted the issue17520-2 branch December 20, 2023 13:45
bmish added a commit to bmish/eslint that referenced this pull request Dec 27, 2023
* main: (25 commits)
  test: ensure that CLI tests run with FlatESLint (eslint#17884)
  fix!: Behavior of CLI when no arguments are passed (eslint#17644)
  docs: Update README
  Revert "feat!: Remove CodePath#currentSegments" (eslint#17890)
  feat!: Update shouldUseFlatConfig and CLI so flat config is default (eslint#17748)
  feat!: Remove CodePath#currentSegments (eslint#17756)
  chore: update dependency markdownlint-cli to ^0.38.0 (eslint#17865)
  feat!: deprecate no-new-symbol, recommend no-new-native-nonconstructor (eslint#17710)
  feat!: check for parsing errors in suggestion fixes (eslint#16639)
  feat!: assert suggestion messages are unique in rule testers (eslint#17532)
  feat!: `no-invalid-regexp` make allowConstructorFlags case-sensitive (eslint#17533)
  fix!: no-sequences rule schema correction (eslint#17878)
  feat!: Update `eslint:recommended` configuration (eslint#17716)
  feat!: drop support for string configurations in flat config array (eslint#17717)
  feat!: Remove `SourceCode#getComments()` (eslint#17715)
  feat!: Remove deprecated context methods (eslint#17698)
  feat!: Swap FlatESLint-ESLint, FlatRuleTester-RuleTester in API (eslint#17823)
  feat!: remove formatters except html, json(-with-metadata), and stylish (eslint#17531)
  feat!: Require Node.js `^18.18.0 || ^20.9.0 || >=21.1.0` (eslint#17725)
  fix: allow circular references in config (eslint#17752)
  ...
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 feature This change adds a new feature to ESLint
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Change Request: Remove deprecated context pass-through SourceCode methods
4 participants