Closed Bug 1294621 Opened 3 years ago Closed 3 years ago

Enable the no-lonely-if rule for eslint

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jaws, Assigned: jaws)

Details

Attachments

(1 file)

no-lonely-if: disallow if statements as the only statement in else blocks

If an if statement is the only statement in the else block, it is often clearer to use an else if form.
Comment on attachment 8780380 [details]
Bug 1294621 - Enable the no-lonely-if rule for eslint.

https://reviewboard.mozilla.org/r/71110/#review68694

::: browser/extensions/pocket/content/panels/js/saved.js:270
(Diff revision 1)
>                      e.preventDefault();
>                      e.stopImmediatePropagation();
>                      inputwrapper.find('.pkt_ext_tag_input').tokenInput('remove', {name:selected.find('p').text()});
>                  }
>              }
> -            else {
> +            else if ($(e.target).parent().hasClass('token-input-input-token')) {

Note that the pocket stuff is in an external repo as well... dunno if you have access, if not, prod mkaply on IRC.

::: toolkit/mozapps/extensions/content/extensions.js:688
(Diff revision 1)
> -      } else {
> +      } else if (gHistory.canGoForward)
> -        if (gHistory.canGoForward)
> -          gHistory.forward();
> +        gHistory.forward();
> -        else
> +      else
> -          gViewController.replaceView(gViewDefault);
> +        gViewController.replaceView(gViewDefault);
> -      }
> +    }

Please add braces around the else if / else clauses here.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6112
(Diff revision 1)
> -      else {
> +      else if (aRequest instanceof Ci.nsIHttpChannel)
> -        if (aRequest instanceof Ci.nsIHttpChannel)
> -          this.downloadFailed(AddonManager.ERROR_NETWORK_FAILURE,
> +        this.downloadFailed(AddonManager.ERROR_NETWORK_FAILURE,
> -                              aRequest.responseStatus + " " +
> +                            aRequest.responseStatus + " " +
> -                              aRequest.responseStatusText);
> +                            aRequest.responseStatusText);
> -        else
> +      else
> -          this.downloadFailed(AddonManager.ERROR_NETWORK_FAILURE, aStatus);
> +        this.downloadFailed(AddonManager.ERROR_NETWORK_FAILURE, aStatus);

The if is braced, so the else if and else should be braced too. In fact, this looks strange...:

```
}
else if ()
  ...
else
  ...
} // <--- what is this closing? The outer if that's not in the patch context? Extra braces would make this more readable.
else {
```

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7512
(Diff revision 1)
> -    else {
> +    else if (!addon.userDisabled)
>        // Only set softDisabled if not already disabled
> -      if (!addon.userDisabled)
> -        addon.softDisabled = val;
> +      addon.softDisabled = val;
> -    }

Also wants moar braces.

::: toolkit/mozapps/extensions/test/xpcshell/test_bug299716.js:108
(Diff revision 1)
> -  } else {
> +  } else if (aItem != null)
> -    if (aItem != null)
> -      do_throw("Addon " + aAddonsEntry.id + " was detected");
> +    do_throw("Addon " + aAddonsEntry.id + " was detected");

Some more braces here.

::: toolkit/mozapps/extensions/test/xpcshell/test_system_update.js:420
(Diff revision 1)
> -  else {
> +  else if (finalState[0])
>      // If the new state is using the profile then that directory will exist.
> -    if (finalState[0])
> -      expectedDirs++;
> +    expectedDirs++;

And here
Attachment #8780380 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/6d1a4462e489
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.