eslint icon indicating copy to clipboard operation
eslint copied to clipboard

Change Request: RuleTester check suggestion fixers

Open DMartens opened this issue 2 years ago • 6 comments

ESLint version

8.12.0

What problem do you want to solve?

The rule tester currently checks if any fixes result in a parser error but it does include suggestion fixers.

What do you think is the correct solution?

I would propose to include suggestion fixes as well by checking each suggestion in isolation.

Participation

  • [X] I am willing to submit a pull request for this change.

Additional comments

No response

DMartens avatar Mar 28 '22 18:03 DMartens

Hi @DMartens, thanks for the issue!

I would propose to include suggestion fixes as well by checking each suggestion in isolation.

Can you please clarify this, I think I understand but just to be sure. The proposal is to check for each suggestion whether applying the suggested fix would produce parsing errors?

mdjermanovic avatar Mar 28 '22 21:03 mdjermanovic

Yes, you understood correctly. I said in isolation as there may be multiple errors each with multiple suggestions. This differs from output as all fixes from all reported errors are applied at once. So there are multiple ways to handle it:

  1. Each suggestion in isolation (my suggestion)
  2. One suggestion per error (this would speed up the parser fail checks as there are less reparses)
  3. Apply all non-overlapping suggestions at once, repeat until none left

I chose (1) as

  • Debuggability: In the other cases you don't know exactly which suggestion caused the error
  • Performance: The advantage of the other is that but the test cases usually are small
  • Simplicity: Easy to implement

DMartens avatar Mar 28 '22 21:03 DMartens

Makes sense to me :+1:

The only downside I see is that the tests will take a bit more time to execute due to the additional parsing, but this seems worth it.

Note that this change requires a RFC, but first let's see what other team members think about this proposal.

  1. Each suggestion in isolation (my suggestion)

This seems like the most correct solution to implement this check. :+1:

mdjermanovic avatar Mar 28 '22 22:03 mdjermanovic

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

github-actions[bot] avatar May 27 '22 22:05 github-actions[bot]

I want to add my support for this request.

Ideally, we would have included this change in a related RFC that has been approved but not implemented yet:

  • https://github.com/eslint/rfcs/pull/84

bmish avatar Dec 09 '22 20:12 bmish

I opened:

  • https://github.com/eslint/rfcs/pull/101 - RFC
  • https://github.com/eslint/eslint/pull/16639 - draft implementation

bmish avatar Dec 10 '22 20:12 bmish