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!: check for parsing errors in suggestion fixes #16639

Merged
merged 4 commits into from Dec 20, 2023

Conversation

bmish
Copy link
Sponsor Member

@bmish bmish commented Dec 10, 2022

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)

Fixes #15735.

RFC (approved): eslint/rfcs#101

In rule tester, check for parsing errors in suggestion fixes, and throw an error if a suggestion yielded invalid output.

This is a breaking change for plugin developers who may have had invalid suggestions.

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

This PR is ready on my side. I'm lining this PR up now so that it's ready for the v9 release.

@bmish bmish requested a review from a team as a code owner December 10, 2022 19:02
@eslint-github-bot eslint-github-bot bot added breaking This change is backwards-incompatible triage An ESLint team member will look at this issue soon feature This change adds a new feature to ESLint labels Dec 10, 2022
@netlify
Copy link

netlify bot commented Dec 10, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 37f87e3
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6570a6878f4a28000829d1e7

@github-actions
Copy link

github-actions bot commented Feb 8, 2023

Oops! It looks like we lost track of this pull request. What do we want to do here? This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Feb 8, 2023
@bmish bmish removed the Stale label Feb 14, 2023
@github-actions
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 Feb 24, 2023
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

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 Mar 3, 2023
@bmish bmish reopened this May 21, 2023
@bmish bmish force-pushed the suggestion-fix-parse-error branch from bb50f15 to 62681aa Compare May 21, 2023 19:56
@bmish bmish removed the Stale label May 21, 2023
@bmish bmish self-assigned this May 21, 2023
@nzakas
Copy link
Member

nzakas commented Jun 20, 2023

@bmish can you provide a status update on this?

@bmish
Copy link
Sponsor Member Author

bmish commented Jun 20, 2023

I believe this is still ready for review. It's one of several breaking changes I'm trying to have lined up ready to go when ESLint v9 comes around.

@nzakas
Copy link
Member

nzakas commented Jun 26, 2023

Okay, we generally ignore PRs that are drafts until they marked as ready for review. Given that this is a breaking change, maybe we want to hold off on reviewing until we're ready for v9?

* main: (285 commits)
  8.55.0
  Build: changelog update for 8.55.0
  chore: upgrade @eslint/js@8.55.0 (eslint#17811)
  chore: package.json update for @eslint/js release
  chore: upgrade @eslint/eslintrc@2.1.4 (eslint#17799)
  feat: importNamePattern option in no-restricted-imports (eslint#17721)
  docs: fix typo `--rules` -> `--rule` (eslint#17806)
  ci: pin Node.js 21.2.0 (eslint#17809)
  chore: fix several `cli` tests to run in the intended flat config mode (eslint#17797)
  docs: remove "Open in Playground" buttons for removed rules (eslint#17791)
  docs: fix correct/incorrect examples of rules (eslint#17789)
  docs: update and fix examples for `no-unused-vars` (eslint#17788)
  docs: add specific stylistic rule for each deprecated rule (eslint#17778)
  chore: remove unused config-extends fixtures (eslint#17781)
  chore: remove formatting/stylistic rules from new rule templates (eslint#17780)
  chore: check rule examples for syntax errors (eslint#17718)
  8.54.0
  Build: changelog update for 8.54.0
  chore: upgrade @eslint/js@8.54.0 (eslint#17773)
  chore: package.json update for @eslint/js release
  ...
@bmish
Copy link
Sponsor Member Author

bmish commented Dec 3, 2023

This PR is ready, except there is a failure with one rule we need to fix, see:

* main:
  fix: suggestion with invalid syntax in no-promise-executor-return rule (eslint#17812)
@mdjermanovic mdjermanovic marked this pull request as draft December 6, 2023 13:01
@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 and removed triage An ESLint team member will look at this issue soon labels Dec 6, 2023
@mdjermanovic
Copy link
Member

Converted back to draft as we keep breaking changes as drafts to prevent accidental merging before we decide to start with v9 prereleases.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

We are going to be removing RuleTester in v9, so can you revert those changes? I'd like to avoid merge conflicts as go through the process of removing RuleTester.

@bmish
Copy link
Sponsor Member Author

bmish commented Dec 6, 2023

@nzakas done!

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! Leaving it in the "Implementing" status in the v9.0.0 project for @nzakas to verify before we move it to "Ready for Merge".

Copy link
Member

@nzakas nzakas 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!

@mdjermanovic mdjermanovic marked this pull request as ready for review December 20, 2023 16:33
@mdjermanovic mdjermanovic merged commit 5aa9c49 into eslint:main Dec 20, 2023
22 checks passed
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: RuleTester check for parsing errors in suggestion fixes
3 participants