[ESLint] Fix ESLint errors/warnings in devtools/client/webconsole/console-commands.js

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Developer Tools: Console
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: linclark, Assigned: gasolin@mozilla.com, Mentored)

Tracking

unspecified
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [btpp-backlog])

MozReview Requests

()

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
This is an easy bug for new contributors.

If you haven’t contributed to Firefox before, follow the steps here to set up your environment: https://developer.mozilla.org/en-US/docs/Tools/Contributing#Getting_set_up

Then, automatically configure ESLint to work with the Firefox specific rules by following the instructions here: https://wiki.mozilla.org/DevTools/CodingStandards

Then you can see the issues that need to be fixed by running

> eslint --no-ignore devtools/client/webconsole/console-commands.js
(Reporter)

Updated

2 years ago
Blocks: 1256948
Priority: -- → P3
(Reporter)

Updated

2 years ago
Whiteboard: [btpp-backlog]

Comment 1

2 years ago
I would like to work on this bug.
(Reporter)

Updated

2 years ago
Assignee: nobody → marek.timr

Comment 2

2 years ago
Created attachment 8732890 [details] [diff] [review]
0001-Bug-1256767-Fix-ESLint-errors-warnings-in-devtools-c.patch

I'm clueless what to do on line 78. What can I return, if there is no panel which generates a promise?
Attachment #8732890 - Flags: review?(lclark)
(Reporter)

Comment 3

2 years ago
Comment on attachment 8732890 [details] [diff] [review]
0001-Bug-1256767-Fix-ESLint-errors-warnings-in-devtools-c.patch

Review of attachment 8732890 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks for the patch! Just a few minor changes and it should be ready.

::: devtools/client/webconsole/console-commands.js
@@ +45,5 @@
>        let toolbox = gDevTools.getToolbox(target);
>  
>        if (!toolbox) {
> +        return gDevTools.showToolbox(target, "inspector")
> +                        .then((shownToolbox) => {

Instead of shownToolbox, let's go with newToolbox.

@@ -78,2 @@
>        panel.hud.jsterm.clearOutput();
> -      return onceMessagesCleared;

Instead of removing this return, what you can do is add "return null" to the two return statements above this one. That will make the error go away.

@@ +85,5 @@
>      description: l10n.lookup("consolecloseDesc"),
>      exec: function(args, context) {
>        return gDevTools.closeToolbox(context.environment.target)
> +                      .then(() => {});
> +      // Don't return a value to GCLI

Now that we don't have the comment at the end, we can probably fit the `.then()` call on the line above. After that, the comment can be placed above the return, like:

> // Don't return a value to GCL      
> return gDevTools.closeToolbox(context.environment.target).then(() => {});

@@ +97,5 @@
>      exec: function(args, context) {
>        const target = context.environment.target;
>        return gDevTools.showToolbox(target, "webconsole")
> +                      .then(() => {});
> +      // Don't return a value to GCLI

See above.
Attachment #8732890 - Flags: review?(lclark) → review-
(Assignee)

Comment 4

2 years ago
steal it
Assignee: marek.timr → gasolin
(Assignee)

Comment 5

2 years ago
Created attachment 8743197 [details]
MozReview Request: Bug 1256767 - Fix ESLint errors/warnings in devtools/client/webconsole/console-commands.js; r?linclark

Review commit: https://reviewboard.mozilla.org/r/47639/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47639/
Attachment #8743197 - Flags: review?(lclark)
(Assignee)

Comment 6

2 years ago
Comment on attachment 8743197 [details]
MozReview Request: Bug 1256767 - Fix ESLint errors/warnings in devtools/client/webconsole/console-commands.js; r?linclark

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47639/diff/1-2/
(Reporter)

Comment 7

2 years ago
Comment on attachment 8743197 [details]
MozReview Request: Bug 1256767 - Fix ESLint errors/warnings in devtools/client/webconsole/console-commands.js; r?linclark

https://reviewboard.mozilla.org/r/47639/#review46717

Thanks, and sorry for the delay in review.

::: devtools/client/webconsole/console-commands.js:47
(Diff revision 2)
>        }
>      },
>      exec: function(args, context) {
>        let target = context.environment.target;
>        let toolbox = gDevTools.getToolbox(target);
> +      let promise = null;

Rather than setting promise to null and then returning the variable regardless, I'd rather get rid of the else clause itself. This should work because the if statement will return early, meaning that the code after it will not be hit. Hopefully that will make the rule pass.
Attachment #8743197 - Flags: review?(lclark)
(Assignee)

Comment 8

2 years ago
Thanks for review. I tried to keep the origin syntax and remove the `else` clause, it will still cause error:

`error  Expected to return a value at the end of this function`


Do you prefer to eslint-disable `consistent-return` in this section, or pick any alternative?
Flags: needinfo?(lclark)
(Reporter)

Comment 9

2 years ago
Maybe this would work:

> toolbox.toggleSplitConsole();
> return null;
Flags: needinfo?(lclark)
(Assignee)

Comment 10

2 years ago
(In reply to Lin Clark [:linclark] (on leave thru May 14) from comment #9)
> Maybe this would work:
> 
> > toolbox.toggleSplitConsole();
> > return null;

Thanks for suggestion, but that will cause `Unexpected 'else' after return (no-else-return)` error.

We need fulfill both this lint and the lint in comment 8
Are you still working on this? If not, please de-assign yourself.
Flags: needinfo?(gasolin)
(Assignee)

Comment 12

2 years ago
will send review again since no other suggestions can pass lint check. 
Currently reviewer might in PTO and does not accept review.
Flags: needinfo?(gasolin)

Comment 13

2 years ago
(In reply to Fred Lin [:gasolin] from comment #12)
> will send review again since no other suggestions can pass lint check. 
> Currently reviewer might in PTO and does not accept review.
You can ask :bgrins or :jlast if :lclark is unavailable.

(In reply to Fred Lin [:gasolin] from comment #10)
> (In reply to Lin Clark [:linclark] (on leave thru May 14) from comment #9)
> > Maybe this would work:
> > 
> > > toolbox.toggleSplitConsole();
> > > return null;
> 
> Thanks for suggestion, but that will cause `Unexpected 'else' after return
> (no-else-return)` error.
> 
> We need fulfill both this lint and the lint in comment 8

      let target = context.environment.target;
      let toolbox = gDevTools.getToolbox(target);

      if (!toolbox) {
        return gDevTools.showToolbox(target, "inspector").then((toolbox) => {
          toolbox.toggleSplitConsole();
        });
      }
      return toolbox.toggleSplitConsole();

Does this solution fix both rules ? It should do the same thing as the old code.
Flags: needinfo?(gasolin)
(Assignee)

Comment 14

2 years ago
Comment on attachment 8743197 [details]
MozReview Request: Bug 1256767 - Fix ESLint errors/warnings in devtools/client/webconsole/console-commands.js; r?linclark

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47639/diff/2-3/
Attachment #8743197 - Flags: review?(lclark)
(Assignee)

Comment 15

2 years ago
Thanks for suggestions! That fixed all lint
Flags: needinfo?(gasolin)
(Reporter)

Updated

2 years ago
Attachment #8743197 - Flags: review?(lclark) → review+
(Reporter)

Comment 16

2 years ago
Comment on attachment 8743197 [details]
MozReview Request: Bug 1256767 - Fix ESLint errors/warnings in devtools/client/webconsole/console-commands.js; r?linclark

https://reviewboard.mozilla.org/r/47639/#review52648

Passes lint and tests, sounds good to me. Thanks!

Updated

2 years ago
Attachment #8732890 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
thanks!
Keywords: checkin-needed
seems this might need a rebase -

applying 8e462d6dbbef
patching file .eslintignore
Hunk #1 FAILED at 108
1 out of 1 hunks FAILED -- saving rejects to file .eslintignore.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

could you take a look, thanks!
Flags: needinfo?(gasolin)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 19

2 years ago
Comment on attachment 8743197 [details]
MozReview Request: Bug 1256767 - Fix ESLint errors/warnings in devtools/client/webconsole/console-commands.js; r?linclark

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47639/diff/3-4/
(Assignee)

Comment 20

2 years ago
Thanks for notice. Patch has been rebased.
Flags: needinfo?(gasolin) → needinfo?(cbook)
thanks done!
Flags: needinfo?(cbook)

Comment 23

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