Enable the no-else-return rule for eslint.

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
Created attachment 8777517 [details]
Bug 1291855 - Enable the no-else-return rule for eslint.

Review commit: https://reviewboard.mozilla.org/r/69062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69062/
Attachment #8777517 - Flags: review?(kmaglione+bmo)
Attachment #8777517 - Flags: review?(jryans)
Attachment #8777517 - Flags: review?(felipc)
Attachment #8777517 - Flags: review?(dtownsend)
Mossop, can you review the .eslintrc change?
Felipe, can you review the toolkit and browser changes?
Kris, can you review the extension changes?
Comment on attachment 8777517 [details]
Bug 1291855 - Enable the no-else-return rule for eslint.

I thought there were devtools changes here but there isn't, so I'm clearing jyrans' review request.
Attachment #8777517 - Flags: review?(jryans)
jryans*
Comment on attachment 8777517 [details]
Bug 1291855 - Enable the no-else-return rule for eslint.

https://reviewboard.mozilla.org/r/69062/#review66140

r=me for the WebExtensions, subprocess, and AOM changes.

::: toolkit/components/extensions/Extension.jsm:1270
(Diff revision 1)
>          });
>        });
> -    } else {
> -      throw Error("installType must be one of: temporary, permanent");
>      }
> +    throw Error("installType must be one of: temporary, permanent");

Please also add `new` keyword here.

::: toolkit/components/extensions/test/mochitest/test_ext_permission_xhr.html:44
(Diff revision 1)
>        if (shouldFail) {
>          return fetch("http://example.org/example.txt").then(failListener, passListener);
> -      } else {
> -        return fetch("http://example.com/example.txt").then(passListener, failListener);
>        }
> +      return fetch("http://example.com/example.txt").then(passListener, failListener);

Hm. I'd rather `/* eslint-disable no-else-return */` for this block. Having the two fetch calls aligned here is useful for seeing what's different.

::: toolkit/mozapps/extensions/content/extensions.js:1646
(Diff revision 1)
>             addon.pendingOperations != AddonManager.PENDING_INSTALL))
>          return "disabled";
>        if (addonType && (addonType.flags & AddonManager.TYPE_SUPPORTS_ASK_TO_ACTIVATE) &&
> -          addon.userDisabled == AddonManager.STATE_ASK_TO_ACTIVATE)
> +          addon.userDisabled == AddonManager.STATE_ASK_TO_ACTIVATE) {
>          return "askToActivate";
> -      else
> +      }

No braces here, please.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1497
(Diff revision 1)
>  });
>  
>  function loadManifestFromFile(aFile, aInstallLocation) {
> -  if (aFile.isFile())
> +  if (aFile.isFile()) {
>      return loadManifestFromZipFile(aFile, aInstallLocation);
> -  else
> +  }

No braces.

::: toolkit/mozapps/extensions/nsBlocklistService.js:1177
(Diff revision 1)
>    getPluginBlocklistState: function(plugin, appVersion, toolkitVersion) {
>      if (AppConstants.platform == "android" ||
>          AppConstants.MOZ_B2G) {
>        return Ci.nsIBlocklistService.STATE_NOT_BLOCKED;
> -    } else {
> -      if (!this._isBlocklistLoaded())
> +    }
> +    if (!this._isBlocklistLoaded()) {

Add-on manager code doesn't use braces for single-line `if`s unless they're part of a chain with a multi-line block.

::: toolkit/mozapps/extensions/test/browser/browser_plugin_enabled_state_locked.js:19
(Diff revision 1)
> -  if (gIsWindows)
> +  if (gIsWindows) {
>      return prefix + "nptest";
> -  else if (gIsLinux)
> +  }
> +  if (gIsLinux) {
>      return prefix + "libnptest";
> -  else
> +  }

No braces. Also, would be nice to convert these to template strings, while you're here.

::: toolkit/mozapps/installer/precompile_cache.js:67
(Diff revision 1)
>             .concat(dir_entries(file.file, "modules", ".js"))
>             .concat(dir_entries(file.file, "modules", ".jsm"));
> -  } else {
> -    throw "Expected a nsIJARURI or nsIFileURL";
>    }
> +  throw "Expected a nsIJARURI or nsIFileURL";

We should convert this to throw a real Error while we're here.
Attachment #8777517 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8777517 [details]
Bug 1291855 - Enable the no-else-return rule for eslint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69062/diff/1-2/
Comment on attachment 8777517 [details]
Bug 1291855 - Enable the no-else-return rule for eslint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69062/diff/2-3/
Comment on attachment 8777517 [details]
Bug 1291855 - Enable the no-else-return rule for eslint.

https://reviewboard.mozilla.org/r/69062/#review66170
Attachment #8777517 - Flags: review?(dtownsend) → review+
Comment on attachment 8777517 [details]
Bug 1291855 - Enable the no-else-return rule for eslint.

https://reviewboard.mozilla.org/r/69062/#review66192

a couple of places here are changing the bracing style for one-liners `if`, which is the style in most of toolkit. I commented on the places that I saw, except for test files.

::: toolkit/components/extensions/ExtensionContent.jsm:531
(Diff revision 3)
>      let readyState = contentWindow.document.readyState;
>      if (readyState == "complete") {
>        return "document_idle";
>      } else if (readyState == "interactive") {
>        return "document_end";
> -    } else {
> -      return "document_start";
>      }
> +    return "document_start";

the `else if` here looks like just a case that this rule didn't pick up. Please fix it too.

::: toolkit/components/extensions/ExtensionManagement.jsm:48
(Diff revision 3)
>      if (this.isTopWindowId(windowId)) {
>        return 0;
>      } else if (windowId == 0) {
>        return -1;
> -    } else {
> -      return windowId;
>      }
> +    return windowId;

same here

::: toolkit/components/extensions/Schemas.jsm:1591
(Diff revision 3)
>        return readJSON(url).then(json => {
>          this.schemaJSON.set(url, json);
>  
>          let data = Services.ppmm.initialProcessData;
>          data["Extension:Schemas"] = this.schemaJSON;
>  
>          Services.ppmm.broadcastAsyncMessage("Schema:Add", {url, schema: json});
>  
>          loadFromJSON(json);
>        });
> -    } else {
> +    }
> -      if (this.loadedUrls.has(url)) {
> +    if (this.loadedUrls.has(url)) {
> -        return;
> +      return;
> -      }
> +    }
> -      this.loadedUrls.add(url);
> +    this.loadedUrls.add(url);

the rule being applied here looks to me to be making the code less legible, since the return is far away and not immediatelly above the end brace.

I'd either add the magic comment to ignore the rule here, or change the code to:

`let foo = readJSON(url).then( ...{`
`  ...`
`});`
`return foo;`

::: toolkit/components/feeds/FeedProcessor.js:1603
(Diff revision 3)
> -    if (!uri)
> +    if (!uri) {
>        return "";
> +    }
>      var prefix = gNamespaces[uri];
> -    if (prefix)
> +    if (prefix) {
>        return prefix + ":";
> -    if (uri.toLowerCase().indexOf("http://backend.userland.com") == 0)
> +    }
> +    if (uri.toLowerCase().indexOf("http://backend.userland.com") == 0) {
>        return "";
> -    else
> +    }

not that I like the no-braces style, but the style on this file is consisntely no-braces for one liners.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:1483
(Diff revision 3)
>     * Returns the localized string for the specified key,
>     * formatted if required.
>     *
>     */
>    _getLocalizedString : function (key, formatArgs) {
> -    if (formatArgs)
> +    if (formatArgs) {

brace style change

::: toolkit/components/prompts/src/nsPrompter.js:124
(Diff revision 3)
>  // Common utils not specific to a particular prompter style.
>  var PromptUtilsTemp = {
>      __proto__ : PromptUtils,
>  
>      getLocalizedString : function (key, formatArgs) {
> -        if (formatArgs)
> +        if (formatArgs) {

brace style change throughout this file

::: toolkit/components/satchel/nsFormAutoComplete.js:594
(Diff revision 3)
>  
>      // Interfaces from idl...
>      searchString : "",
>      errorDescription : "",
>      get defaultIndex() {
> -        if (this.entries.length == 0)
> +        if (this.entries.length == 0) {

brace style change

::: toolkit/content/widgets/dialog.xml:59
(Diff revision 3)
>                  onset="this._configureButtons(val); return val;"/>
>  
>        <property name="defaultButton">
>          <getter>
>          <![CDATA[
> -          if (this.hasAttribute("defaultButton"))
> +          if (this.hasAttribute("defaultButton")) {

brace style change

::: toolkit/modules/Geometry.jsm:67
(Diff revision 3)
>  };
>  
>  (function() {
>    function takePointOrArgs(f) {
>      return function(arg1, arg2) {
> -      if (arg2 === undefined)
> +      if (arg2 === undefined) {

brace style change
Attachment #8777517 - Flags: review?(felipc) → review+
Comment on attachment 8777517 [details]
Bug 1291855 - Enable the no-else-return rule for eslint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69062/diff/3-4/
Comment on attachment 8777517 [details]
Bug 1291855 - Enable the no-else-return rule for eslint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69062/diff/4-5/

Comment 14

2 years ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6265328c999c
Enable the no-else-return rule for eslint. r=Felipe,kmag,mossop
Thanks for the quick reviews!

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6265328c999c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.