Autofix rules #21

sindresorhus · 2016-02-27T05:29:09Z

Would be useful to support autofixing for some of the rules. Not all are possible.

http://eslint.org/docs/developer-guide/working-with-rules#contextreport

no-skip-test (just remove the skip modifier)
no-only-test (just remove the only modifier)
no-identical-title (append Discussion #1, no-cb-test rule #2, etc, to the title)
no-skip-assert (just remove the skip modifier)

Can't think of any way to reliably fix the other rules, but input welcome!

The text was updated successfully, but these errors were encountered:

jfmengels · 2016-02-27T14:51:06Z

👍 Great idea.

Not sure about no-identical-title though, as they won't really make things more readable in the source code (only when matching AVA's output to a test). Don't forget that the rule can also be triggered because there is no title.

sindresorhus · 2016-03-03T07:10:21Z

Not sure about no-identical-title though, as they won't really make things more readable in the source code (only when matching AVA's output to a test).

Appending a #n does make it clear it's testing the same thing just split up for readability, but I don't feel strongly about it, and is a pretty edge-case anyways.

jamestalmage · 2016-03-22T18:43:08Z

W/regard to the no-skip-* rules, I think auto fixing those is problematic. It will likely make the tests fail. If I am running xo --fix to clear up some whitespace / semicolon mistakes - I probably don't want you messing with my tests/assertions.

I don't think we should auto-fix anything that modifies code behavior (unless maybe the user sets some other CLI option or environment variable? FIX_AVA=true xo --fix.

One exception to the above. I think removing only is fine. Users should never check in .only tests, but .skip is acceptable in a number of situations (i.e. somebody has submitted a PR with a failing test but not the fix itself).

sindresorhus · 2016-03-22T18:46:49Z

But without it will make XO fail. So it will still fail.

jamestalmage · 2016-03-22T18:52:51Z

Does --fix only fix errors, or warnings too?

sindresorhus · 2016-03-23T13:09:00Z

Warnings too.

florianb · 2017-02-23T08:16:47Z

@sindresorhus: i would like to give it a try if it is still relevant.

sindresorhus · 2017-02-23T08:26:56Z

@florianb Sure. Go ahead. Seems like we couldn't agree on all of these yet, but you could start with autofixing for no-only-test :) There might also be other rules not mentioned here that could potentially have autofixing. Feel free to look into that too.

florianb · 2017-02-23T08:28:23Z

Great - i am sure a PR will stimulate discussions.. 😄

florianb · 2017-03-08T21:01:42Z

Hey everybody, i am stuck testing the fixResult.output since error.actual does not get printed. If i am not wrong this needs to be fixed in https://github.com/jfmengels/eslint-ava-rule-tester - i would be very happy to fix that if a contributor could check my assumption and approve that a PR for that would be welcome.

Thank you very much! 💝

sindresorhus · 2017-03-20T06:50:22Z

@florianb Just apply the fix mention in jfmengels/eslint-ava-rule-tester#5 locally to your node_modules and you should be able to debug it for now.

florianb · 2017-03-31T09:53:41Z

@sindresorhus i guess this fix is not applicable since eslint forbids the modification of the AST. While it it would (probably) be possible to rename .only into something different, i can't remove it without causing the following exception:

Rule should not modify AST.

Actual:
[object Object]

Expected:
[object Object]

assertASTDidntChange (node_modules/eslint/lib/testers/rule-tester.js:406:24)
...

I opened a corresponding SO-question without helpful responses. I guess i could start a bounty to raise attention but i can't think of any solution overcoming this barrier.

I am really sorry that i just got stuck again. 😞

mightyiam · 2017-03-31T10:06:34Z

Keep it up, Florian. The light is ahead of the curve.

…

On Fri, Mar 31, 2017, 12:53 Florian Breisch ***@***.***> wrote: @sindresorhus <https://github.com/sindresorhus> i guess this fix is not applicable since eslint forbids the modification of the AST. While it it would (probably) be possible to rename .only into something different, i can't remove it without causing the following exception: Rule should not modify AST. Actual: [object Object] Expected: [object Object] assertASTDidntChange (node_modules/eslint/lib/testers/rule-tester.js:406:24) ... I opened a corresponding SO-question <https://stackoverflow.com/questions/43043746/how-to-modify-the-javascript-ast-in-an-eslint-rule-fix> without helpful responses. I guess i could start a bounty to raise attention but i can't think of any solution overcoming this barrier. I am really sorry that i just got stuck again. 😞 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#21 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAmyx8zhN7eb-uHslydazj9H6hF7W9umks5rrM0lgaJpZM4HkYpp> .

sindresorhus · 2017-03-31T13:53:21Z

@florianb Can you submit a WIP PR so we can take a look and maybe help?

florianb · 2017-03-31T13:54:02Z

@sindresorhus - of course. Wait a minute.. 😄

florianb · 2017-03-31T14:04:16Z

@sindresorhus - created #173

florianb · 2017-11-27T08:42:49Z

Okaydo - heading on to implement the no-skip-test-fix.

florianb · 2017-12-07T17:55:57Z

🎺 I'm heading over to implement the fixer for no-skip-assert.

In thought of a fix for no-identical-title i'd propose adding with ${superhero()}-powers instead of numbers.

sindresorhus · 2017-12-07T22:16:32Z

In thought of a fix for no-identical-title i'd propose adding with ${superhero()}-powers instead of numbers.

Haha. Sounds funny in theory, but I think people will not appreciate our humor.

jfmengels · 2017-12-07T23:29:44Z

🎺 I'm heading over to implement the fixer for no-skip-assert.

Cool !

As for no-identical-title, joke aside, I am against having a fix for it. The title should IMO be a description of the expected behavior and conditions for that test case, and all of them grouped together should form a specification of the tested function(ality). It should be fixed by the developer so that the test has a useful title.

That said, these tests could have be autofixed:

no-async-without-await
no-duplicate-modifiers
no-todo-implementation (debatable)
no-unknown-modifiers (debatable)

sindresorhus · 2017-12-07T23:44:33Z

As for no-identical-title, joke aside, I am against having a fix for it. The title should IMO be a description of the expected behavior and conditions for that test case, and all of them grouped together should form a specification of the tested function(ality). It should be fixed by the developer so that the test has a useful title.

I agree.

florianb · 2017-12-08T09:54:45Z

Sure - so just to check if i got you all right 😅:

No superpowers for identical titles
No fix for identical titles
The former list of three fixes this issue aimed for magically expanded to provide also fixes for:
no-async-without-await
no-duplicate-modifiers
no-todo-implementation
no-unknown-modifiers

sindresorhus · 2017-12-08T18:40:45Z

Correct

sindresorhus · 2017-12-08T18:41:40Z

But I don't think we should do a fixer for no-todo-implementation. What if the user accidentally wrote a test body and then we just remove it. Better for them to just fix it manually.

jfmengels · 2017-12-08T18:45:43Z

For no-todo-implementation, I thought we could remove the todo modifier. Yeah we definitely don't want to remove the test body.

…

On Fri, Dec 8, 2017, 19:41 Sindre Sorhus ***@***.***> wrote: But I don't think we should do no-todo-implementation. What if the user accidentally wrote a test body and then we just remove it. Better for them to just fix it manually. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#21 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADsK5Ftfe3rfgROJPe33AMJuAK7Cf1ajks5s-YLlgaJpZM4HkYpp> .

sindresorhus · 2017-12-08T18:48:50Z

Oh ok, makes more sense. I still don't think it should have a fixer though.

florianb · 2017-12-11T10:17:25Z

Well - then thanks for extending the scope.. 😆

I'll ignore the no-todo-implementation for now.

mmkal · 2020-04-26T15:54:38Z

@sindresorhus piggy-backing on this issue: IMO no-only-test should not have an autofixer. See eslint/eslint#7549 (comment) for why and eslint/eslint#7549 (comment) for an explicit note that this kind of fix is not recommended by the eslint team.

It can be actively harmful to auto-fix no-only-test (and no-skip-test) - here's a scenario the current behaviour encourages:

I write a test which is failing, and I want to debug it
I add test.only in the code as a quick way to run just this test and no others in the file
vscode instantly deletes what I wrote(!) when I press save
I write it again, and this time suppress the lint warning
I debug and fix the test
I am forgetful. Everything looks normal so I check the code with test.only and the suppression in
Tests pass - but only one of them is running
This test gap goes unnoticed in master for months

By comparison, here's what happens with jest/no-disabled-tests

I write a test which is failing, and I want to debug it
I add test.only in the code as a quick way to run just this test and no others in the file
vscode shows me a red squiggly. I'm fine with that. Only my test runs; I debug and fix it.
Since I can see the red squiggly, I'm much less likely to forget about the .only, so probably I remove it before checking in. But even if I don't, CI will catch it and tell me about a lint error.
All tests keep running in master

sindresorhus · 2020-04-26T16:07:35Z

@mmkal Yes, we plan to drop the auto-fixer for those rules. See: #281 (comment)

sindresorhus added the enhancement label Feb 27, 2016

sindresorhus added the help wanted label Feb 28, 2016

florianb mentioned this issue Feb 28, 2017

RuleTesters AssertionError.actual is not being displayed jfmengels/eslint-ava-rule-tester#5

Closed

florianb mentioned this issue Nov 17, 2017

Add fixer for the no-only-test rule #173

Merged

florianb mentioned this issue Nov 27, 2017

Add fixer for no-skip-test rule #183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autofix rules #21

Autofix rules #21

sindresorhus commented Feb 27, 2016 •

edited

jfmengels commented Feb 27, 2016

sindresorhus commented Mar 3, 2016

jamestalmage commented Mar 22, 2016

sindresorhus commented Mar 22, 2016

jamestalmage commented Mar 22, 2016

sindresorhus commented Mar 23, 2016

florianb commented Feb 23, 2017

sindresorhus commented Feb 23, 2017 •

edited

florianb commented Feb 23, 2017

florianb commented Mar 8, 2017

sindresorhus commented Mar 20, 2017

florianb commented Mar 31, 2017

mightyiam commented Mar 31, 2017 via email

sindresorhus commented Mar 31, 2017

florianb commented Mar 31, 2017

florianb commented Mar 31, 2017

florianb commented Nov 27, 2017

florianb commented Dec 7, 2017

sindresorhus commented Dec 7, 2017

jfmengels commented Dec 7, 2017

sindresorhus commented Dec 7, 2017

florianb commented Dec 8, 2017 •

edited

sindresorhus commented Dec 8, 2017 •

edited

sindresorhus commented Dec 8, 2017 •

edited

jfmengels commented Dec 8, 2017 via email

sindresorhus commented Dec 8, 2017

florianb commented Dec 11, 2017 •

edited

mmkal commented Apr 26, 2020 •

edited

sindresorhus commented Apr 26, 2020

Autofix rules #21

Autofix rules #21

Comments

sindresorhus commented Feb 27, 2016 • edited

jfmengels commented Feb 27, 2016

sindresorhus commented Mar 3, 2016

jamestalmage commented Mar 22, 2016

sindresorhus commented Mar 22, 2016

jamestalmage commented Mar 22, 2016

sindresorhus commented Mar 23, 2016

florianb commented Feb 23, 2017

sindresorhus commented Feb 23, 2017 • edited

florianb commented Feb 23, 2017

florianb commented Mar 8, 2017

sindresorhus commented Mar 20, 2017

florianb commented Mar 31, 2017

mightyiam commented Mar 31, 2017 via email

sindresorhus commented Mar 31, 2017

florianb commented Mar 31, 2017

florianb commented Mar 31, 2017

florianb commented Nov 27, 2017

florianb commented Dec 7, 2017

sindresorhus commented Dec 7, 2017

jfmengels commented Dec 7, 2017

sindresorhus commented Dec 7, 2017

florianb commented Dec 8, 2017 • edited

sindresorhus commented Dec 8, 2017 • edited

sindresorhus commented Dec 8, 2017 • edited

jfmengels commented Dec 8, 2017 via email

sindresorhus commented Dec 8, 2017

florianb commented Dec 11, 2017 • edited

mmkal commented Apr 26, 2020 • edited

sindresorhus commented Apr 26, 2020

sindresorhus commented Feb 27, 2016 •

edited

sindresorhus commented Feb 23, 2017 •

edited

florianb commented Dec 8, 2017 •

edited

sindresorhus commented Dec 8, 2017 •

edited

sindresorhus commented Dec 8, 2017 •

edited

florianb commented Dec 11, 2017 •

edited

mmkal commented Apr 26, 2020 •

edited