Closed Bug 1256767 Opened 5 years ago Closed 4 years ago

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

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: linclark, Assigned: gasolin, Mentored)

References

Details

(Whiteboard: [btpp-backlog])

Attachments

(1 file, 1 obsolete file)

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
Blocks: 1256948
Priority: -- → P3
Whiteboard: [btpp-backlog]
I would like to work on this bug.
Assignee: nobody → marek.timr
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)
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-
steal it
Assignee: marek.timr → gasolin
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/
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)
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)
Maybe this would work:

> toolbox.toggleSplitConsole();
> return null;
Flags: needinfo?(lclark)
(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)
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)
(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)
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)
Thanks for suggestions! That fixed all lint
Flags: needinfo?(gasolin)
Attachment #8743197 - Flags: review?(lclark) → review+
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!
Attachment #8732890 - Attachment is obsolete: true
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)
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/
Thanks for notice. Patch has been rebased.
Flags: needinfo?(gasolin) → needinfo?(cbook)
thanks done!
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/fb00a4b41a9f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.