-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add fixer for the no-only-test
rule
#173
Conversation
Can you also mark it |
rules/no-only-test.js
Outdated
fix: fixer => { | ||
const range = getTestModifier(node, 'only').range; | ||
range[0] -= 1; | ||
return fixer.remove(getTestModifier(node, 'only')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do:
const range = getTestModifier(node, 'only').range.slice();
range[0] -= 1;
return fixer.removeRange(range);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
rules/no-only-test.js
Outdated
@@ -2,6 +2,20 @@ | |||
const visitIf = require('enhance-visitors').visitIf; | |||
const createAvaRule = require('../create-ava-rule'); | |||
|
|||
const getTestModifier = function (node, mod) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use arrow-function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And move it to util.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
rules/no-only-test.js
Outdated
return getTestModifier(node.object, mod); | ||
} | ||
|
||
return undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
Okay @sindresorhus - i finally made the changes, apologies for the delay. Moving |
Hi @florianb, thanks for your work on this :) I would like to add three cases to the tests. The third one fails and currently creates incorrect JS code ( {
code: header + 'test\n .only(t => { t.pass(); });',
output: header + 'test\n (t => { t.pass(); });',
errors
},
{
code: header + 'test .only(t => { t.pass(); });',
output: header + 'test (t => { t.pass(); });',
errors
},
{
code: header + 'test.\n only(t => { t.pass(); });',
output: header + 'test\n (t => { t.pass(); });',
errors
} That said, I'm actually against having this rule be fixable 🤔 |
Hi @jfmengels - thanks for suggesting the test-cases, they are perfectly valid! I absolutely understand your opinion, to be honest, i picked that rule since i wanted to help out and the But to add something to the discussion - i use fixes to make my current work compliant before pushing to the remote. |
That can be solved in the editor plugins though, like AtomLinter/linter-eslint#795. |
Ah, I didn't know about that, that's great to hear. |
And in atom-linter-xo, I can disable the auto-fix for it when fixing on save. |
Aye - thanks, going to add the test. ⏳ |
@jfmengels - i added additional tests using tabs instead of spaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @florianb :)
No description provided.