Update: add allowElseIf option to no-else-return (fixes #9228) #9229

graingert · 2017-09-04T13:07:54Z

No description provided.

eslintbot · 2017-09-04T13:07:56Z

Thanks for the pull request, @graingert! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

mention-bot · 2017-09-04T13:07:56Z

@graingert, thanks for your PR! By analyzing the history of the files in this pull request, we identified @xdumaine, @not-an-aardvark and @kaicataldo to be potential reviewers.

eslintbot · 2017-09-04T17:50:14Z

Thanks for the pull request, @graingert! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2017-09-04T18:03:09Z

Thanks for the pull request, @graingert! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2017-09-04T18:05:39Z

Thanks for the pull request, @graingert! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2017-09-04T18:06:56Z

LGTM

graingert · 2017-09-04T18:07:39Z

lib/rules/no-else-return.js

@@ -208,15 +205,9 @@ module.exports = {
                return;
            }

-            for (consequents = []; node.type === "IfStatement"; node = node.alternate) {


no idea why this was added, looks like it's only there to break else if reporting?

graingert · 2017-09-04T18:08:27Z

@platinumazure I got rid of quite a bit of code that I'm not really sure what it's for. (except to introduce the bug I reported).

See #9229 (diff)

eslintbot · 2017-09-04T20:34:45Z

LGTM

eslintbot · 2017-09-04T20:37:47Z

LGTM

eslintbot · 2017-09-04T20:59:25Z

LGTM

eslintbot · 2017-09-05T10:08:16Z

LGTM

platinumazure · 2017-09-05T16:11:19Z

@eslint/eslint-team Please take a look at this PR. I'm no longer sure this bugfix should actually be a default behavior change: maybe this should actually be behind an option, based on some of the changes I'm seeing in ESLint core files that became necessary as a result of the rule change (and which, IMO, became less readable as a result). Personally, I think there's an argument to be made for allowing a user to have else if (even when not strictly necessary) for readability reasons. Anyone else have any thoughts?

not-an-aardvark · 2017-09-05T18:15:41Z

Personally, I think the new behavior seems more correct. I wouldn't mind adding an option for backwards compatibility (since I suspect this pattern is fairly common), but it seems like the rule is currently not enforcing what it's supposed to enforce.

graingert · 2017-09-11T13:13:28Z

@not-an-aardvark what's the conclusion on this?

eslintbot · 2017-09-11T13:13:53Z

LGTM

not-an-aardvark · 2017-09-12T02:34:54Z

I'm not sure -- there hasn't been any discussion about it outside of what you see on this issue. Sorry about the delay.

@platinumazure Do you think we should add this to the TSC agenda so we can get it resolved?

platinumazure · 2017-09-12T02:38:48Z

Yes, sorry, that would be good. I just have a feeling that the change could cause a lot of people to be confused without it being behind an option, but it shouldn't be my call. Sorry for delaying this.

eslintbot · 2017-09-29T01:50:15Z

LGTM

not-an-aardvark

LGTM. Thanks!

graingert · 2017-09-29T10:06:17Z

@not-an-aardvark woop! What's the next step?

platinumazure

LGTM, thanks very much for putting this behind an option.

kaicataldo

Thanks for working on this! Would you mind adding documentation for the new option?

graingert · 2017-09-29T19:12:52Z

I was hoping to get away without it... Thomas Grainger

…

On 29 September 2017 at 20:11, Kai Cataldo ***@***.***> wrote: ***@***.**** requested changes on this pull request. Thanks for working on this! Would you mind adding documentation for the new option? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9229 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZQTGQsQr3GdBI248OcMCDmYuLuxuvOks5snUDqgaJpZM4PL5cr> .

kaicataldo · 2017-09-29T20:27:23Z

But it's the most important part! Helps everyone know about and use your new feature :)

eslintbot · 2017-09-30T12:53:15Z

LGTM

eslintbot · 2017-09-30T12:54:57Z

LGTM

eslintbot · 2017-09-30T12:55:58Z

LGTM

eslintbot · 2017-09-30T12:58:06Z

LGTM

graingert · 2017-09-30T13:02:11Z

@not-an-aardvark @kaicataldo added docs

kaicataldo

LGTM. Thanks!

graingert · 2017-09-30T16:51:46Z

@kaicataldo thanks for the review! What's the next step?

not-an-aardvark · 2017-09-30T18:28:49Z

We just did a release on Friday, so we're waiting to merge PRs until we determine whether a patch release will be necessary on Monday (see the release issue for updates). So this will probably get merged sometime around Monday, assuming no one else requests changes on it.

graingert force-pushed the no-else-return-else-if branch from 820ec31 to bd808ff Compare September 4, 2017 18:03

graingert force-pushed the no-else-return-else-if branch from bd808ff to 597a960 Compare September 4, 2017 18:05

graingert force-pushed the no-else-return-else-if branch from 597a960 to 1625b0a Compare September 4, 2017 18:06

graingert commented Sep 4, 2017

View reviewed changes

graingert force-pushed the no-else-return-else-if branch from 1625b0a to aa92f08 Compare September 4, 2017 20:34

graingert force-pushed the no-else-return-else-if branch from 56a0e8a to a77c4eb Compare September 4, 2017 20:59

graingert changed the title ~~WIP: support else if in no-else-return #9228~~ support else if in no-else-return #9228 Sep 4, 2017

graingert force-pushed the no-else-return-else-if branch from a77c4eb to 797bd4a Compare September 5, 2017 10:08

not-an-aardvark added bug ESLint is working incorrectly 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 5, 2017

graingert force-pushed the no-else-return-else-if branch from 797bd4a to 76f2568 Compare September 11, 2017 13:13

not-an-aardvark added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Sep 12, 2017

not-an-aardvark approved these changes Sep 29, 2017

View reviewed changes

platinumazure approved these changes Sep 29, 2017

View reviewed changes

kaicataldo suggested changes Sep 29, 2017

View reviewed changes

graingert added 7 commits September 30, 2017 13:40

Update: remove untested check from no-else-return

80ef72f

Update: Add test and move existing tests for eslint#9228

58c1feb

Fix: support else if in return-else-if

4f5679d

Update: set allowElseIf: false for eslint-config-eslint no-else-return

4ee4438

Fix: eslint autofix core files

175fe43

Fix: manual fix eslint core files

5c5bd0d

Fix: manual fix eslint test files

26b317a

graingert force-pushed the no-else-return-else-if branch from ae7e209 to 8fb263d Compare September 30, 2017 12:53

graingert force-pushed the no-else-return-else-if branch from 8fb263d to 9e224b7 Compare September 30, 2017 12:54

graingert force-pushed the no-else-return-else-if branch from 9e224b7 to ab66b89 Compare September 30, 2017 12:55

Docs: add no-else-return allowElseIf docs

7cd25c6

graingert force-pushed the no-else-return-else-if branch from ab66b89 to 7cd25c6 Compare September 30, 2017 12:58

kaicataldo approved these changes Sep 30, 2017

View reviewed changes

not-an-aardvark merged commit 1167638 into eslint:master Oct 2, 2017

eslint-deprecated bot locked and limited conversation to collaborators Apr 1, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: add allowElseIf option to no-else-return (fixes #9228) #9229

Update: add allowElseIf option to no-else-return (fixes #9228) #9229

graingert commented Sep 4, 2017

eslintbot commented Sep 4, 2017

mention-bot commented Sep 4, 2017

eslintbot commented Sep 4, 2017

eslintbot commented Sep 4, 2017

eslintbot commented Sep 4, 2017

eslintbot commented Sep 4, 2017

graingert Sep 4, 2017

graingert commented Sep 4, 2017 •

edited

eslintbot commented Sep 4, 2017

eslintbot commented Sep 4, 2017

eslintbot commented Sep 4, 2017

eslintbot commented Sep 5, 2017

platinumazure commented Sep 5, 2017 •

edited

not-an-aardvark commented Sep 5, 2017

graingert commented Sep 11, 2017

eslintbot commented Sep 11, 2017

not-an-aardvark commented Sep 12, 2017

platinumazure commented Sep 12, 2017

eslintbot commented Sep 29, 2017

not-an-aardvark left a comment

graingert commented Sep 29, 2017

platinumazure left a comment

kaicataldo left a comment

graingert commented Sep 29, 2017 via email

kaicataldo commented Sep 29, 2017 •

edited

eslintbot commented Sep 30, 2017

eslintbot commented Sep 30, 2017

eslintbot commented Sep 30, 2017

eslintbot commented Sep 30, 2017

graingert commented Sep 30, 2017

kaicataldo left a comment

graingert commented Sep 30, 2017

not-an-aardvark commented Sep 30, 2017

Update: add allowElseIf option to no-else-return (fixes #9228) #9229

Update: add allowElseIf option to no-else-return (fixes #9228) #9229

Conversation

graingert commented Sep 4, 2017

eslintbot commented Sep 4, 2017

mention-bot commented Sep 4, 2017

eslintbot commented Sep 4, 2017

eslintbot commented Sep 4, 2017

eslintbot commented Sep 4, 2017

eslintbot commented Sep 4, 2017

graingert Sep 4, 2017

Choose a reason for hiding this comment

graingert commented Sep 4, 2017 • edited

eslintbot commented Sep 4, 2017

eslintbot commented Sep 4, 2017

eslintbot commented Sep 4, 2017

eslintbot commented Sep 5, 2017

platinumazure commented Sep 5, 2017 • edited

not-an-aardvark commented Sep 5, 2017

graingert commented Sep 11, 2017

eslintbot commented Sep 11, 2017

not-an-aardvark commented Sep 12, 2017

platinumazure commented Sep 12, 2017

eslintbot commented Sep 29, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

graingert commented Sep 29, 2017

platinumazure left a comment

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

graingert commented Sep 29, 2017 via email

kaicataldo commented Sep 29, 2017 • edited

eslintbot commented Sep 30, 2017

eslintbot commented Sep 30, 2017

eslintbot commented Sep 30, 2017

eslintbot commented Sep 30, 2017

graingert commented Sep 30, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

graingert commented Sep 30, 2017

not-an-aardvark commented Sep 30, 2017

graingert commented Sep 4, 2017 •

edited

platinumazure commented Sep 5, 2017 •

edited

kaicataldo commented Sep 29, 2017 •

edited