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

RESOLVED FIXED in Firefox 49

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: linclark, Assigned: gasolin, Mentored)

Tracking

unspecified
Firefox 49
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [btpp-backlog])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

3 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

3 years ago
Blocks: 1256948
Priority: -- → P3
Reporter

Updated

3 years ago
Whiteboard: [btpp-backlog]

Comment 1

3 years ago
I would like to work on this bug.
Reporter

Updated

3 years ago
Assignee: nobody → marek.timr

Comment 2

3 years ago
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

3 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

3 years ago
steal it
Assignee: marek.timr → gasolin
Assignee

Comment 6

3 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

3 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

3 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

3 years ago
Maybe this would work:

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

Comment 10

3 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

3 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

3 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

3 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

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

Updated

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

Comment 16

3 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

3 years ago
Attachment #8732890 - Attachment is obsolete: true
Assignee

Comment 17

3 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)
Assignee

Comment 19

3 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

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

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fb00a4b41a9f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.