Closed
Bug 1256767
Opened 8 years ago
Closed 8 years ago
[ESLint] Fix ESLint errors/warnings in devtools/client/webconsole/console-commands.js
Categories
(DevTools :: Console, defect, P3)
DevTools
Console
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
Reporter | ||
Updated•8 years ago
|
Whiteboard: [btpp-backlog]
Comment 1•8 years ago
|
||
I would like to work on this bug.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → marek.timr
Comment 2•8 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•8 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 5•8 years ago
|
||
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•8 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•8 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•8 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•8 years ago
|
||
Maybe this would work:
> toolbox.toggleSplitConsole();
> return null;
Flags: needinfo?(lclark)
Assignee | ||
Comment 10•8 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
Comment 11•8 years ago
|
||
Are you still working on this? If not, please de-assign yourself.
Flags: needinfo?(gasolin)
Assignee | ||
Comment 12•8 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•8 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•8 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•8 years ago
|
||
Thanks for suggestions! That fixed all lint
Flags: needinfo?(gasolin)
Reporter | ||
Updated•8 years ago
|
Attachment #8743197 -
Flags: review?(lclark) → review+
Reporter | ||
Comment 16•8 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•8 years ago
|
Attachment #8732890 -
Attachment is obsolete: true
Comment 18•8 years ago
|
||
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•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 19•8 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•8 years ago
|
||
Thanks for notice. Patch has been rebased.
Flags: needinfo?(gasolin) → needinfo?(cbook)
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fb00a4b41a9f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•