Open sindresorhus opened 8 years ago
:+1: 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.
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.
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).
But without it will make XO fail. So it will still fail.
Does --fix
only fix errors, or warnings too?
Warnings too.
@sindresorhus: i would like to give it a try if it is still relevant.
@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.
Great - i am sure a PR will stimulate discussions.. 😄
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! 💝
@florianb Just apply the fix mention in https://github.com/jfmengels/eslint-ava-rule-tester/issues/5 locally to your node_modules and you should be able to debug it for now.
@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. 😞
Keep it up, Florian. The light is ahead of the curve.
On Fri, Mar 31, 2017, 12:53 Florian Breisch notifications@github.com 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 https://github.com/avajs/eslint-plugin-ava/issues/21#issuecomment-290670396, or mute the thread https://github.com/notifications/unsubscribe-auth/AAmyx8zhN7eb-uHslydazj9H6hF7W9umks5rrM0lgaJpZM4HkYpp .
@florianb Can you submit a WIP PR so we can take a look and maybe help?
@sindresorhus - of course. Wait a minute.. 😄
@sindresorhus - created #173
Okaydo - heading on to implement the no-skip-test
-fix.
🎺 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.
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.
:trumpet: 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)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.
Sure - so just to check if i got you all right 😅:
no-async-without-await
no-duplicate-modifiers
no-todo-implementation
no-unknown-modifiers
Correct
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.
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 notifications@github.com 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 https://github.com/avajs/eslint-plugin-ava/issues/21#issuecomment-350339727, or mute the thread https://github.com/notifications/unsubscribe-auth/ADsK5Ftfe3rfgROJPe33AMJuAK7Cf1ajks5s-YLlgaJpZM4HkYpp .
Oh ok, makes more sense. I still don't think it should have a fixer though.
Well - then thanks for extending the scope.. 😆
I'll ignore the no-todo-implementation
for now.
@sindresorhus piggy-backing on this issue: IMO no-only-test
should not have an autofixer. See https://github.com/eslint/eslint/issues/7549#issuecomment-383002000 for why and https://github.com/eslint/eslint/issues/7549#issuecomment-383010918 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:
test.only
and the suppression inBy comparison, here's what happens with jest/no-disabled-tests
.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.@mmkal Yes, we plan to drop the auto-fixer for those rules. See: https://github.com/avajs/eslint-plugin-ava/issues/281#issuecomment-587137623
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 theskip
modifier)no-only-test
(just remove theonly
modifier)no-identical-title
(append #1, #2, etc, to the title)no-skip-assert
(just remove theskip
modifier)Can't think of any way to reliably fix the other rules, but input welcome!