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

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

Merged
merged 4 commits into from
Nov 24, 2017

Conversation

florianb
Copy link
Contributor

No description provided.

@florianb florianb mentioned this pull request Mar 31, 2017
4 tasks
@sindresorhus
Copy link
Member

Can you also mark it fixable in the docs, like done here https://github.com/sindresorhus/eslint-plugin-unicorn#rules

fix: fixer => {
const range = getTestModifier(node, 'only').range;
range[0] -= 1;
return fixer.remove(getTestModifier(node, 'only'));
Copy link
Member

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

@@ -2,6 +2,20 @@
const visitIf = require('enhance-visitors').visitIf;
const createAvaRule = require('../create-ava-rule');

const getTestModifier = function (node, mod) {
Copy link
Member

Choose a reason for hiding this comment

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

Use arrow-function.

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

return getTestModifier(node.object, mod);
}

return undefined;
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

@florianb florianb changed the title WIP: Draft fix for no-only Fix for no-only Nov 2, 2017
@florianb
Copy link
Contributor Author

florianb commented Nov 2, 2017

Okay @sindresorhus - i finally made the changes, apologies for the delay.

Moving getTestModifier to util made it necessary using a named function instead of an arrow function. Let me know if further changes are needed. 🍸

@jfmengels
Copy link
Contributor

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 (test.(t => { t.pass(); });).

		{
			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 🤔
I use autofix a lot now (manually), for things like formatting (especially since I started using prettier and a lot of core rules became autofixable), and some people even have it run automatically on save. Having this rule be fixable means that you can not run the autofix (or even save your file?) while you want to run some tests with only.

@florianb
Copy link
Contributor Author

florianb commented Nov 17, 2017

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 no-only-rule wasn't discussed controversial in #21. I will wait with any further activities until the members found a consent on this.

But to add something to the discussion - i use fixes to make my current work compliant before pushing to the remote.

@sindresorhus
Copy link
Member

Having this rule be fixable means that you can not run the autofix (or even save your file?) while you want to run some tests with only.

That can be solved in the editor plugins though, like AtomLinter/linter-eslint#795.

@jfmengels
Copy link
Contributor

Ah, I didn't know about that, that's great to hear.
I think most people will not use that, but it's probably worth trying adding it. If it bothers a lot of people, we can always explain in the docs how to disable the autofixing in the editor.
👍 for me then, pending the addition of the tests that I put above.

@sindresorhus
Copy link
Member

sindresorhus commented Nov 22, 2017

And in atom-linter-xo, I can disable the auto-fix for it when fixing on save.

@florianb
Copy link
Contributor Author

Aye - thanks, going to add the test. ⏳

@florianb
Copy link
Contributor Author

@jfmengels - i added additional tests using tabs instead of spaces.
@sindresorhus - done, even though i guess you'll have to refine my style for your final pleasure. 😄

Copy link
Contributor

@jfmengels jfmengels 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 @florianb :)

@sindresorhus sindresorhus changed the title Fix for no-only Add fixer for the no-only-test rule Nov 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants