Skip to content

Commit

Permalink
add rule prefer-reject
Browse files Browse the repository at this point in the history
  • Loading branch information
ganimomer committed Aug 23, 2015
1 parent d2e35b1 commit 37f146e
Show file tree
Hide file tree
Showing 7 changed files with 144 additions and 7 deletions.
6 changes: 3 additions & 3 deletions README.md
Expand Up @@ -34,7 +34,8 @@ Finally, enable all of the rules that you would like to use.
"lodash3/matches-prop-shorthand": 1,
"lodash3/prefer-chain": 1,
"lodash3/preferred-alias": 1,
"lodash3/no-single-chain": 1
"lodash3/no-single-chain": 1,
"lodash3/prefer-reject": 1
}
}
```
Expand All @@ -46,9 +47,8 @@ Finally, enable all of the rules that you would like to use.
* [preferred-alias](docs/rules/preferred-alias.md): Preferred aliases
* [prefer-chain](docs/rules/prefer-chain.md): Prefer chain over nested lodash calls
* [no-single-chain](docs/rules/no-single-chain.md): Prevent chaining syntax for single method, e.g. `_(x).map().value()`
* [prefer-reject](docs/rules/prefer-reject.md): Prefer `_.reject` over filter with `!(expression)` or `x.prop1 !== value`

# TODO
* `prefer-reject` prefer `reject` over filter with `_.negate`, `!(expression)` or `x.prop1.prop2 !== value`


# License
Expand Down
30 changes: 30 additions & 0 deletions docs/rules/prefer-reject.md
@@ -0,0 +1,30 @@
# Prefer reject

When using _.filter with a negative condition, it could improve readability by switching to _.reject

## Rule Details

This rule takes no arguments.

The following patterns are considered warnings:

```js

_.filter(arr, function(x) { return x.a !== b});

_.filter(arr, function(x) {return !x.isSomething})
```

The following patterns are not considered warnings:

```js

var x = _.filter(arr, function(x) {return !x.a && p});

This comment has been minimized.

Copy link
@cafesanu

cafesanu Sep 6, 2015

I think you meant reject

This comment has been minimized.

Copy link
@ganimomer

ganimomer Sep 7, 2015

Author Contributor

No, I actually meant filter. The reasoning being, if the condition is more complicated than just a negation, it shouldn't show up as an error in this rule.


var x = _.filter(arr, function(x) {return !f(x)}; // The function f could take multiple arguments, e.g. parseInt

This comment has been minimized.

Copy link
@cafesanu

cafesanu Sep 6, 2015

Same here. I think you meant reject

This comment has been minimized.

Copy link
@ganimomer

ganimomer Sep 7, 2015

Author Contributor

Same here. While this is only a negation, it's equivalent to _.reject(arr, function(x) {return f(x)}).
It can't be simplified to _.reject(arr, f) because we don't know how many arguments f takes.
for example, _.reject(arr, parseInt) is probably not what you have in mind, as it would take the second argument (the index) as the base of the int.
Since there's not a lot to be gained by rejecting in this case, the rule allows it.

```
## When Not To Use It
If you do not want to enforce using `_.reject`, you should not use this rule.
6 changes: 4 additions & 2 deletions index.js
Expand Up @@ -6,13 +6,15 @@ module.exports = {
'prefer-chain': require('./lib/rules/prefer-chain'),
'prop-shorthand': require('./lib/rules/prop-shorthand'),
'matches-prop-shorthand': require('./lib/rules/matches-prop-shorthand'),
'no-single-chain': require('./lib/rules/no-single-chain')
'no-single-chain': require('./lib/rules/no-single-chain'),
'prefer-reject': require('./lib/rules/prefer-reject')
},
rulesConfig: {
'preferred-alias': 0,
'prefer-chain': 0,
'prop-shorthand': 0,
'matches-prop-shorthand': 0,
'no-single-chain': 0
'no-single-chain': 0,
'prefer-reject': 0
}
};
46 changes: 46 additions & 0 deletions lib/rules/prefer-reject.js
@@ -0,0 +1,46 @@
/**
* @fileoverview Rule to check if a call to filter should be a call to reject
*/
'use strict';

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

module.exports = function (context) {
var esUtil = require('../util/esUtil');
var aliases = require('../util/aliases');

function isFilter(node) {
return aliases.isAliasOfMethod('filter', esUtil.getMethodName(node));
}

function isParamNegation(exp, firstParamName) {
return exp && exp.type === 'UnaryExpression' && exp.operator === '!' && esUtil.isMemberExpOfArg(exp.argument, firstParamName);
}

function isParamNotEqEq(exp, firstParamName) {
return exp && exp.type === 'BinaryExpression' && exp.operator === '!==' &&
(esUtil.isMemberExpOfArg(exp.left, firstParamName) || esUtil.isMemberExpOfArg(exp.right, firstParamName));
}

function isNegativeExpressionFunction(func) {
var firstLine = esUtil.getFirstFunctionLine(func);
var firstParamName = esUtil.getFirstParamName(func);
return esUtil.isReturnStatement(firstLine) &&
(isParamNegation(firstLine.argument, firstParamName) ||
isParamNotEqEq(firstLine.argument, firstParamName));
}

return {
CallExpression: function (node) {
try {
if (isFilter(node) && isNegativeExpressionFunction(esUtil.getLodashIteratee(node))) {
context.report(node, 'Prefer _.reject over negative condition');
}
} catch (e) {
context.report(node, 'Error executing rule: ' + e + ' ' + e.stack);
}
}
};
};
7 changes: 6 additions & 1 deletion lib/util/aliases.js
Expand Up @@ -41,8 +41,9 @@ var CHAINABLE = [
'shuffle', 'slice', 'sort', 'sortBy', 'sortByAll', 'sortByOrder', 'splice', 'spread', 'take', 'takeRight', 'takeRightWhile', 'takeWhile', 'tap', 'throttle', 'thru', 'times', 'toArray',
'toPlainObject', 'transform', 'union', 'uniq', 'unshift', 'unzip', 'unzipWith', 'values', 'valuesIn', 'where', 'without', 'wrap', 'xor', 'zip', 'zipObject', 'zipWith'];

var ALL_ALIASES = _.assign({}, ALIASES, WRAPPER_ALIASES);
function expandAlias(method) {
return method in ALIASES ? ALIASES[method].concat(method) : [method];
return method in ALL_ALIASES ? ALL_ALIASES[method].concat(method) : [method];
}

function expandAliases(methods) {
Expand All @@ -53,8 +54,12 @@ var CHAINABLE_ALIASES = expandAliases(CHAINABLE);

var supportsProp = expandAliases(property);

function isAliasOfMethod(method, suspect) {
return _.includes(expandAlias(method), suspect);
}

module.exports = {
isAliasOfMethod: isAliasOfMethod,
ALIASES: ALIASES,
CHAINABLE: CHAINABLE,
CHAINABLE_ALIASES: CHAINABLE_ALIASES,
Expand Down
26 changes: 25 additions & 1 deletion lib/util/esUtil.js
Expand Up @@ -29,6 +29,18 @@ function isLodashWrapper(node) {
return node && node.type === 'CallExpression' && node.callee.type === 'MemberExpression' && isChainable(node) && isLodashWrapper(node.callee.object);
}

function getLodashIteratee(node) {
if (isLodashCall(node)) {
return node.arguments && node.arguments[1];
} else if (isLodashWrapper(getCaller(node))) {
return node.arguments && node.arguments[0];
}
}

function getFirstFunctionLine(node) {
return node && node.type === 'FunctionExpression' && _.get(node, 'body.body[0]');
}

/**
* is member access that is not by index or by a variable e.g.
* a.b or a['b']
Expand All @@ -47,12 +59,24 @@ function isMemberExpOfArg(node, argName) {
return node.type === 'MemberExpression' && node.object && node.object.name === argName && isPropAccess(node);
}

function getFirstParamName(func) {
return _.get(func, 'params[0].name');
}

function isReturnStatement(exp) {
return exp && exp.type === 'ReturnStatement';
}

module.exports = {
isLodashCall: isLodashCall,
getMethodName: getMethodName,
isLodashChainStart: isLodashChainStart,
isChainable: isChainable,
isLodashWrapper: isLodashWrapper,
getCaller: getCaller,
isMemberExpOfArg: isMemberExpOfArg
isMemberExpOfArg: isMemberExpOfArg,
getLodashIteratee: getLodashIteratee,
getFirstFunctionLine: getFirstFunctionLine,
getFirstParamName: getFirstParamName,
isReturnStatement: isReturnStatement
};
30 changes: 30 additions & 0 deletions tests/lib/rules/prefer-reject.js
@@ -0,0 +1,30 @@
'use strict';

// ------------------------------------------------------------------------------
// Requirements
// ------------------------------------------------------------------------------

var rule = require('../../../lib/rules/prefer-reject');
var RuleTester = require('eslint').RuleTester;

// ------------------------------------------------------------------------------
// Tests
// ------------------------------------------------------------------------------

var ruleTester = new RuleTester();
var ruleError = {message: 'Prefer _.reject over negative condition'};
ruleTester.run('prefer-reject', rule, {
valid: [{
code: 'var x = _.filter(arr, function(x) {return !x.a && p})'
}],
invalid: [{
code: '_(arr).map(f).filter(function(x) {return !x.isSomething})',
errors: [ruleError]
}, {
code: '_.filter(arr, function(x) { return x.a !== b})',
errors: [ruleError]
}, {
code: '_.filter(arr, function(x) {return !x.isSomething})',
errors: [ruleError]
}]
});

0 comments on commit 37f146e

Please sign in to comment.