New: Add allowImplicit option to array-callback-return (fixes #8539) #9344
Conversation
|
LGTM |
|
@jamescdavis, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @vitorbal and @kaicataldo to be potential reviewers. |
not-an-aardvark
added
enhancement
|
Thanks for the pull request! It looks like the issue that this addresses isn't accepted yet (it still has the "evaluating" label). We generally merge enhancements only if the team reaches consensus that they should be added (by having 3 👍s from team members on the issue, and having one team member willing to champion the proposal). It's fine that you've submitted a pull request early -- I just wanted to let you know that it might take longer for this to get reviewed/merged, because the corresponding issue isn't accepted yet. |
ilyavolodin
added
accepted
|
Issue is now marked as accepted. |
| { 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 }, | ||
|
|
There was a problem hiding this comment.
could you please add more invalid tests, e.g:
{ code: "foo.some(function() {})", errors: [error] },
{ code: "foo.some(function() {})", options, errors: [error] },
|
seems ready to go. someone is willing to review this? @eslint/eslint-team |
docs/rules/array-callback-return.md
Outdated
|
|
||
| This rule has an object option: | ||
|
|
||
| * `"allowImplicit": false` (default) disallows implicitly returning undefined with a return; statement. |
There was a problem hiding this comment.
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.
| @@ -14,11 +14,22 @@ const rule = require("../../../lib/rules/array-callback-return"), | |||
|
|
|||
| const ruleTester = new RuleTester(); | |||
|
|
|||
| const options = [{ allowImplicit: true }]; | |||
There was a problem hiding this comment.
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 } |
There was a problem hiding this comment.
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.
|
closing, reopen to restart build. |
|
@aladdin-add, thank you for adding those additional tests and responding to code review! |
docs/rules/array-callback-return.md
Outdated
|
|
||
| This rule has an object option: | ||
|
|
||
| * `"allowImplicit": false` (default) disallows implicitly returning `undefined` with a `return` statement. |
There was a problem hiding this comment.
I understand that the description is speaking about when this option is set to false, but I can imagine how someone else reading this for the first time might not read it that way. For clarity, can we update it so something like the following?
"allowImplicit": false(default) When set totrue, allows implicitly returningundefinedwith areturnstatement.
|
|
||
| // options: { allowImplicit: true } | ||
| { code: "foo.every(() => { return; })", options: allowImplicitOptions, parserOptions: { ecmaVersion: 6 } }, | ||
| { code: "foo.every(function() { if (a) return; else return; })", options: allowImplicitOptions }, |
There was a problem hiding this comment.
"foo.every(function() { if (a) return; else return; })"
It could be nice to turn this into
"foo.every(function() { if (a) return; else return a; })"
as a test both for when there are implicit and explicity returns and also to make sure the behavior is correct for explicit returns when allowImplicit is `true.
|
Had two more small suggestions, but otherwise this LGTM! |
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Implements: #8539
What changes did you make? (Give an overview)
Added an
allowImplicitoption toarray-callback-returnthat is consistent with theallowImplicitoption ingetter-return.Is there anything you'd like reviewers to focus on?
Please double-check this implementation is consistent with
getter-return'sallowImplicit.