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.

Sign up for GitHub

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

New: Add allowImplicit option to array-callback-return (fixes #8539) #9344

Merged
merged 9 commits into from
Dec 21, 2017
Merged

New: Add allowImplicit option to array-callback-return (fixes #8539) #9344

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e88c1b5
New: Add allowImplicit option to array-callback-return (fixes #8539)
jamescdavis Sep 23, 2017
9525db7
Chore: add tests
aladdin-add Dec 15, 2017
a83ba9d
Update array-callback-return.md
aladdin-add Dec 19, 2017
4407058
chore: rename options
aladdin-add Dec 19, 2017
d5c37b0
Update array-callback-return.js
aladdin-add Dec 19, 2017
b01deb0
Update array-callback-return.js
aladdin-add Dec 19, 2017
7bb4bf7
Update array-callback-return.js
aladdin-add Dec 19, 2017
ed79336
Improve allowImplicit option description
jamescdavis Dec 21, 2017
a6a606f
Test explicit return when "allowImplicit": true
jamescdavis Dec 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions docs/rules/array-callback-return.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,21 @@ var foo = Array.from(nodes, function(node) {
var bar = foo.map(node => node.getAttribute("id"));
```

## Options

This rule has an object option:

* `"allowImplicit": false` (default) disallows implicitly returning undefined with a return; statement.
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Can undefined and return; be wrapped in backticks so it's clear that they're code snippets? Also, return probably doesn't need the semicolon.


Examples of **correct** code for the `{ "allowImplicit": true }` option:

```js
/*eslint array-callback-return: ["error", { allowImplicit: true }]*/
var undefAllTheThings = myArray.map(function(item) {
return;
});
```

## Known Limitations

This rule checks callback functions of methods with the given names, *even if* the object which has the method is *not* an array.
Expand Down
18 changes: 16 additions & 2 deletions lib/rules/array-callback-return.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,23 @@ module.exports = {
recommended: false
},

schema: []
schema: [
{
type: "object",
properties: {
allowImplicit: {
type: "boolean"
}
},
additionalProperties: false
}
]
},

create(context) {

const options = context.options[0] || { allowImplicit: false };

let funcInfo = {
upper: null,
codePath: null,
Expand Down Expand Up @@ -208,7 +221,8 @@ module.exports = {
if (funcInfo.shouldCheck) {
funcInfo.hasReturn = true;

if (!node.argument) {
// if allowImplicit: false, should also check node.argument
if (!options.allowImplicit && !node.argument) {
context.report({
node,
message: "{{name}} expected a return value.",
Expand Down
33 changes: 33 additions & 0 deletions tests/lib/rules/array-callback-return.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,22 @@ const rule = require("../../../lib/rules/array-callback-return"),

const ruleTester = new RuleTester();

const options = [{ allowImplicit: true }];
Copy link
Member

Choose a reason for hiding this comment

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

This can be made clearer by naming it allowImplicitOptions or something similar. I know that prevents the use of the shorthand object syntax below, but it makes it unclear what this is if it's just called options. It almost seems like it's the default!


ruleTester.run("array-callback-return", rule, {
valid: [

// options: { allowImplicit: false }
Copy link
Member

Choose a reason for hiding this comment

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

I'd love to see at least one test that explicitly tests with { allowImplicit: false } as a safeguard against regressions and to make sure we're exercising all code paths.

"Array.from(x, function() { return true; })",
"Int32Array.from(x, function() { return true; })",

// options: { allowImplicit: true }
{ code: "Array.from(x, function() { return; })", options },
{ code: "Int32Array.from(x, function() { return; })", options },

"Arrow.from(x, function() {})",

// options: { allowImplicit: false }
"foo.every(function() { return true; })",
"foo.filter(function() { return true; })",
"foo.find(function() { return true; })",
Expand All @@ -28,17 +39,39 @@ ruleTester.run("array-callback-return", rule, {
"foo.reduceRight(function() { return true; })",
"foo.some(function() { return true; })",
"foo.sort(function() { return 0; })",

// options: { allowImplicit: true }
{ code: "foo.every(function() { return; })", options },
{ code: "foo.filter(function() { return; })", options },
{ code: "foo.find(function() { return; })", options },
{ code: "foo.findIndex(function() { return; })", options },
{ code: "foo.map(function() { return; })", options },
{ code: "foo.reduce(function() { return; })", options },
{ code: "foo.reduceRight(function() { return; })", options },
{ code: "foo.some(function() { return; })", options },
{ code: "foo.sort(function() { return; })", options },

"foo.abc(function() {})",
"every(function() {})",
"foo[every](function() {})",
"var every = function() {}",
{ code: "foo[`${every}`](function() {})", parserOptions: { ecmaVersion: 6 } },
{ code: "foo.every(() => true)", parserOptions: { ecmaVersion: 6 } },

// options: { allowImplicit: false }
{ code: "foo.every(() => { return true; })", parserOptions: { ecmaVersion: 6 } },
"foo.every(function() { if (a) return true; else return false; })",
"foo.every(function() { switch (a) { case 0: bar(); default: return true; } })",
"foo.every(function() { try { bar(); return true; } catch (err) { return false; } })",
"foo.every(function() { try { bar(); } finally { return true; } })",

// options: { allowImplicit: true }
{ code: "foo.every(() => { return; })", options, parserOptions: { ecmaVersion: 6 } },
{ code: "foo.every(function() { if (a) return; else return; })", options },
{ code: "foo.every(function() { switch (a) { case 0: bar(); default: return; } })", options },
{ code: "foo.every(function() { try { bar(); return; } catch (err) { return; } })", options },
{ code: "foo.every(function() { try { bar(); } finally { return; } })", options },

Copy link
Member

Choose a reason for hiding this comment

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

could you please add more invalid tests, e.g:

{ code: "foo.some(function() {})", errors: [error] },
{ code: "foo.some(function() {})", options, errors: [error] },

"foo.every(function(){}())",
"foo.every(function(){ return function() { return true; }; }())",
"foo.every(function(){ return function() { return; }; })",
Expand Down