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

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

Conversation

jamescdavis
Copy link
Contributor

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 allowImplicit option to array-callback-return that is consistent with the allowImplicit option in getter-return.

Is there anything you'd like reviewers to focus on?

Please double-check this implementation is consistent with getter-return's allowImplicit.

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

@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 not-an-aardvark added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules labels Sep 23, 2017
@not-an-aardvark
Copy link
Member

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 ilyavolodin added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Nov 2, 2017
@ilyavolodin
Copy link
Member

ilyavolodin commented Nov 2, 2017

Issue is now marked as accepted.

Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

sorry for the delay! just left a comment.

{ 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] },

@aladdin-add
Copy link
Member

seems ready to go. someone is willing to review this? @eslint/eslint-team


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.

@@ -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.

@aladdin-add
Copy link
Member

closing, reopen to restart build.

@aladdin-add aladdin-add reopened this Dec 19, 2017
@jamescdavis
Copy link
Contributor Author

@aladdin-add, thank you for adding those additional tests and responding to code review!


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.

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 to true, allows implicitly returning undefined with a return statement.


// options: { allowImplicit: true }
{ code: "foo.every(() => { return; })", options: allowImplicitOptions, parserOptions: { ecmaVersion: 6 } },
{ code: "foo.every(function() { if (a) return; else return; })", options: allowImplicitOptions },
Copy link
Member

Choose a reason for hiding this comment

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

"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.

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 🐦 1 🗿 👍

@kaicataldo
Copy link
Member

Had two more small suggestions, but otherwise this LGTM!

Copy link
Member

@kaicataldo kaicataldo 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 for contributing to ESLint!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants