Add `no-fn-reference-in-iterator` rule - fixes #91 #97

SamVerschueren · 2017-07-05T20:31:14Z

Trying to fix #91 which prevents passing a function directly to an iterator method.

I think that we should also support reduce and reduceRight. Not sure about sort. These are somewhat different though because they accept multiple arguments so should be handled separately.

Just let me know if they should be supported as well.

kevva · 2017-07-05T20:49:09Z

I definitely think sort should be allowed. I often pass in other modules directly in the callback. Not sure about reduce though.

Otherwise I think the PR looks good 👍

SamVerschueren · 2017-07-05T20:54:11Z

Don't think sort should be in the list either. It always has 2 arguments.

kevva · 2017-07-05T21:00:21Z

Also, should we handle closures? Might be overdoing it...

// invalid
[1, 2, 3].map(fn({unicorn: true}));

// valid
[1, 2, 3].map(x => fn({unicorn: true})(x));

SamVerschueren · 2017-07-05T21:07:03Z

Good idea. Think we should yeah!

SamVerschueren · 2017-07-06T19:21:25Z

@sindresorhus any feedback on reduce and reduceRight?

sindresorhus · 2017-07-06T19:19:56Z

docs/rules/no-function-iterator.md

+# Prevents passing a function directly to iterator methods
+
+Prevents passing a function directly to an iterator method to make it more clear with the function accepts.
+


The intro should include why it's a useful rule. See: #91 (comment) I don't think most realize the issue with doing this.

sindresorhus · 2017-07-06T19:21:07Z

rules/no-function-iterator.js

@@ -0,0 +1,34 @@
+'use strict';
+const iteratorMethods = [


Should be a Set.

sindresorhus · 2017-07-06T19:23:26Z

docs/rules/no-function-iterator.md

@@ -0,0 +1,105 @@
+# Prevents passing a function directly to iterator methods
+
+Prevents passing a function directly to an iterator method to make it more clear with the function accepts.


Don't repeat the title in the text here.

sindresorhus · 2017-07-06T19:24:15Z

rules/no-function-iterator.js

+
+const create = context => ({
+	CallExpression: node => {
+		const sourcecode = context.getSourceCode();


Getting the source code is expensive. We should only do it when we actually need it.

sindresorhus · 2017-07-06T19:25:14Z

Wonder if we could use AST selectors for this: http://eslint.org/docs/developer-guide/selectors

sindresorhus · 2017-07-06T19:30:43Z

any feedback on reduce and reduceRight?

reduce() and reduceRight() doesn't make sense, as you always use all their arguments.

sindresorhus · 2017-07-06T19:32:16Z

I think the rule name could be clearer. Not exactly sure what though. Something like no-function-directly-in-iterator, but that's too long.

SamVerschueren · 2017-07-06T19:52:32Z

I was able to do it like this

const iteratorMethods = [
	'filter',
	'map',
	'forEach',
	'every',
	'filter',
	'find',
	'findIndex',
	'some'
];

const selector = 'CallExpression[callee.property.name=/^' + iteratorMethods.join('|') + '$/]';

That way I can drop the isIteratorMethod function. Don't really have a strong preference for one of them.

I think the rule name could be clearer

Wasn't sure about it either. Can't really come up with something that really fits it.

no-function-ref-iterator
no-function-ref-in-iterator
no-fn-ref-in-iterator
no-fn-directly-in-iterator

SamVerschueren · 2017-07-06T19:58:51Z

docs/rules/no-function-iterator.md

@@ -0,0 +1,133 @@
+# Prevents passing a function directly to iterator methods
+
+Suppose you have a `unicorn` module:


I rewrote the example a bit. Feel free to provide feedback on wording/style/...

Looks good!

sindresorhus · 2017-07-06T19:59:16Z

rules/no-function-iterator.js

+		return arg.name;
+	}
+
+	const sourcecode = context.getSourceCode();


This variable feels moot, just use it directly below.

sindresorhus · 2017-07-06T20:00:36Z

That way I can drop the isIteratorMethod function. Don't really have a strong preference for one of them.

Nah, I prefer what you already have.

sindresorhus · 2017-07-06T20:04:26Z

Maybe we just go verbose no-function-reference-in-iterator, unless @kevva can come up with something better.

sindresorhus · 2017-07-06T20:16:05Z

Actually, let's go with no-fn-reference-in-iterator. fn is a common function abbreviation.

SamVerschueren · 2017-07-06T20:31:01Z

I think I processed all the feedback

kevva · 2017-07-06T22:12:51Z

reduce() and reduceRight() doesn't make sense, as you always use all their arguments.

Not necessarily true since they have index and array too, although it might be an edge case.

Maybe we just go verbose no-function-reference-in-iterator, unless @kevva can come up with something better.

I think no-fn-reference-in-iterator is good too. Had a look at what ESLint uses themselves to see if we could stay consistent with their naming, but they're using both function and func in their rule names.

SamVerschueren · 2017-07-07T09:32:38Z

I prefer fn above func to be honest.

sindresorhus · 2017-07-07T10:04:51Z

Not necessarily true since they have index and array too, although it might be an edge case.

Oh, this is embarrassing. I didn't know that. Never used those arguments. I guess they make sense to include after all then.

I prefer fn above func to be honest.

Me too.

sindresorhus · 2017-07-07T12:34:47Z

Just a thought. Should we allow this if the function used is defined in the same scope?

Should be make an exception for built-in types? This is a very common pattern [true, 1, 2].filter(Boolean).

SamVerschueren · 2017-07-07T19:10:41Z

Should we allow this if the function used is defined in the same scope?

I think we shouldn't. It's easy to overlook something and someone can also add a second argument to the function without really noticing this caveat.

Should we make an exception for built-in types?

We could, but I'd suggest using a whitelist. parseInt is also built-in but you don't want someone to write ['1', '2', '3'].map(parseInt).

sindresorhus · 2017-07-07T19:49:10Z

I think we shouldn't. It's easy to overlook something and someone can also add a second argument to the function without really noticing this caveat.

Yeah, you're probably right.

We could, but I'd suggest using a whitelist. parseInt is also built-in but you don't want someone to write ['1', '2', '3'].map(parseInt).

Actually, I can only think of Boolean as an actual use-case. Maybe we just add that first and add others later as needed.

sindresorhus · 2017-07-10T15:25:51Z

Good stuff Sam. This one will be very useful :)

SamVerschueren · 2017-07-10T16:01:32Z

Woop woop, thanks for the feedback :)

add no-function-iterator rule - fixes sindresorhus#91

aae66a2

sindresorhus changed the title ~~add no-function-iterator rule - fixes #91~~ Add no-function-iterator rule - fixes #91 Jul 6, 2017

support closures

d6e0b3a

sindresorhus requested changes Jul 6, 2017

View reviewed changes

process feedback

c791de6

SamVerschueren commented Jul 6, 2017

View reviewed changes

sindresorhus reviewed Jul 6, 2017

View reviewed changes

SamVerschueren added 2 commits July 6, 2017 22:21

tweaks

84204c1

rename rule

51b823e

SamVerschueren changed the title ~~Add no-function-iterator rule - fixes #91~~ Add no-fn-reference-in-iterator rule - fixes #91 Jul 6, 2017

add reduce and reduceRight

73cbb01

add Boolean as valid function reference

e953bb9

sindresorhus approved these changes Jul 10, 2017

View reviewed changes

sindresorhus merged commit c11cc5d into sindresorhus:master Jul 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `no-fn-reference-in-iterator` rule - fixes #91 #97

Add `no-fn-reference-in-iterator` rule - fixes #91 #97

SamVerschueren commented Jul 5, 2017

kevva commented Jul 5, 2017 •

edited

SamVerschueren commented Jul 5, 2017

kevva commented Jul 5, 2017 •

edited

SamVerschueren commented Jul 5, 2017

SamVerschueren commented Jul 6, 2017

sindresorhus Jul 6, 2017

sindresorhus Jul 6, 2017

sindresorhus Jul 6, 2017

sindresorhus Jul 6, 2017

sindresorhus commented Jul 6, 2017

sindresorhus commented Jul 6, 2017

sindresorhus commented Jul 6, 2017

SamVerschueren commented Jul 6, 2017

SamVerschueren Jul 6, 2017

sindresorhus Jul 6, 2017

sindresorhus Jul 6, 2017

sindresorhus commented Jul 6, 2017

sindresorhus commented Jul 6, 2017

sindresorhus commented Jul 6, 2017

SamVerschueren commented Jul 6, 2017

kevva commented Jul 6, 2017

SamVerschueren commented Jul 7, 2017

sindresorhus commented Jul 7, 2017

sindresorhus commented Jul 7, 2017

SamVerschueren commented Jul 7, 2017

sindresorhus commented Jul 7, 2017

sindresorhus commented Jul 10, 2017

SamVerschueren commented Jul 10, 2017

		# Prevents passing a function directly to iterator methods

		Prevents passing a function directly to an iterator method to make it more clear with the function accepts.

		@@ -0,0 +1,105 @@
		# Prevents passing a function directly to iterator methods

		Prevents passing a function directly to an iterator method to make it more clear with the function accepts.

		@@ -0,0 +1,133 @@
		# Prevents passing a function directly to iterator methods

		Suppose you have a `unicorn` module:

Add no-fn-reference-in-iterator rule - fixes #91 #97

Add no-fn-reference-in-iterator rule - fixes #91 #97

Conversation

SamVerschueren commented Jul 5, 2017

kevva commented Jul 5, 2017 • edited

SamVerschueren commented Jul 5, 2017

kevva commented Jul 5, 2017 • edited

SamVerschueren commented Jul 5, 2017

SamVerschueren commented Jul 6, 2017

sindresorhus Jul 6, 2017

Choose a reason for hiding this comment

sindresorhus Jul 6, 2017

Choose a reason for hiding this comment

sindresorhus Jul 6, 2017

Choose a reason for hiding this comment

sindresorhus Jul 6, 2017

Choose a reason for hiding this comment

sindresorhus commented Jul 6, 2017

sindresorhus commented Jul 6, 2017

sindresorhus commented Jul 6, 2017

SamVerschueren commented Jul 6, 2017

SamVerschueren Jul 6, 2017

Choose a reason for hiding this comment

sindresorhus Jul 6, 2017

Choose a reason for hiding this comment

sindresorhus Jul 6, 2017

Choose a reason for hiding this comment

sindresorhus commented Jul 6, 2017

sindresorhus commented Jul 6, 2017

sindresorhus commented Jul 6, 2017

SamVerschueren commented Jul 6, 2017

kevva commented Jul 6, 2017

SamVerschueren commented Jul 7, 2017

sindresorhus commented Jul 7, 2017

sindresorhus commented Jul 7, 2017

SamVerschueren commented Jul 7, 2017

sindresorhus commented Jul 7, 2017

sindresorhus commented Jul 10, 2017

SamVerschueren commented Jul 10, 2017

Add `no-fn-reference-in-iterator` rule - fixes #91 #97

Add `no-fn-reference-in-iterator` rule - fixes #91 #97

kevva commented Jul 5, 2017 •

edited

kevva commented Jul 5, 2017 •

edited