Closed Bug 1256771 Opened 5 years ago Closed 3 years ago

[ESLint] Fix ESLint issues in devtools/client/webconsole/hudservice.js

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1436076

People

(Reporter: linclark, Assigned: Towkir, Mentored)

References

Details

(Whiteboard: [btpp-backlog])

Attachments

(2 files, 2 obsolete files)

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/hudservice.js
Blocks: 1256948
Priority: -- → P3
Whiteboard: [btpp-backlog]
Attached patch 1256771.patchSplinter Review
Hi, Can you please review the patch.Thanks.
Attachment #8739348 - Flags: review?(lclark)
This looks good. I'm just going to run it through our test server to make sure. If the tests pass, this will be good to go.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af16cd22a0c4
Comment on attachment 8739348 [details] [diff] [review]
1256771.patch

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

It looks like there are some test failures which need to be resolved before we land this. You can see what tests are failing by clicking on that link and clicking the orange dt8. A list of tests will show up at the bottom. You can try running the tests locally using ./mach test <path/to/test>
Attachment #8739348 - Flags: review?(lclark) → review-
Comment on attachment 8739348 [details] [diff] [review]
1256771.patch

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

::: devtools/client/webconsole/hudservice.js
@@ +349,5 @@
>     *
>     * @return object
>     *         A promise for the initialization.
>     */
> +  init: function WC_init() {

You can remove the additional function name here and elsewhere in the file (WC_init in this case). It used to be needed to get good stack traces but it isn't anymore.  And there's cases where it's causing a line wrap to avoid the 80 char limit so I'd rather they all get deleted while you are at it.  Thanks!
Hi, 
Can I take this one ? Looks like it's been inactive for a while.
Flags: needinfo?(lclark)
Yes, that sounds good. Thanks for offering.
Flags: needinfo?(lclark)
Attached patch hudservice_eslint_fix.patch (obsolete) — Splinter Review
Hi Brian & Lin,
I have tried to fix some of the errors and now there are some remaining about which I am little confused. Can you please help on some errors ? 

> 28:1   error  Line 28 exceeds the maximum line length of 90            max-len
If I use multiline strings there is another eslint errors, but I think another way should be out there except for using multiline strings
> 121:15  error  'hudId' is defined but never used             no-unused-vars
Shall I remove this ? if so, then how ?
> 174:11  error  'deferred' is defined but never used          no-unused-vars
If I remove this, how ?
> 219:16  error  'BrowserConsole' is already declared in the upper scope  
> no-shadow
what to do with this ?
> 407:24  error  Expected to return a value at the end of this function   
> consistent-return
Shall I add a 'return' at the starting of line (in the patch) 417 ? 
> 417:1   error  Line 417 exceeds the maximum line length of 90        
> max-len
Well, this is not a string. How do I write this in multiple line ? 

Please provide your valuable suggestion on this, and If I have done anything wrong in the patch, correct me please .

(The patch is just a diff file, so it contains no patch header, after your comments, I will update and attach the final one)

Thanks
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Attachment #8783327 - Flags: feedback?(lclark)
Attachment #8783327 - Flags: feedback?(bgrinstead)
(In reply to [:Towkir] Ahmed from comment #7)
> Created attachment 8783327 [details] [diff] [review]
> hudservice_eslint_fix.patch
> 
> Hi Brian & Lin,
> I have tried to fix some of the errors and now there are some remaining
> about which I am little confused. Can you please help on some errors ? 
> 
> > 28:1   error  Line 28 exceeds the maximum line length of 90            max-len
> If I use multiline strings there is another eslint errors, but I think
> another way should be out there except for using multiline strings

Not a fan of us having to do this just for eslint, but you could split into two lines:

const BROWSER_CONSOLE_WINDOW_FEATURES =
  "chrome,titlebar,toolbar,centerscreen,resizable,dialog=no";

> > 121:15  error  'hudId' is defined but never used             no-unused-vars
> Shall I remove this ? if so, then how ?

You should be able to remove it by doing [,hud] instead

> > 174:11  error  'deferred' is defined but never used          no-unused-vars
> If I remove this, how ?

If it's unused you should be able to just remove it

> > 219:16  error  'BrowserConsole' is already declared in the upper scope  
> > no-shadow
> what to do with this ?

Rename the argument BrowserConsole passed into the arrow function as something else, like browserConsole.

> > 407:24  error  Expected to return a value at the end of this function   
> > consistent-return
> Shall I add a 'return' at the starting of line (in the patch) 417 ? 

You should be able to reformat that as:

      browserWin.BrowserViewSourceOfDocument({
        URL: SourceURL,
        lineNumber: SourceLine
      });
      return;

> > 417:1   error  Line 417 exceeds the maximum line length of 90        
> > max-len
> Well, this is not a string. How do I write this in multiple line ? 

You can just split it up like:

    this.gViewSourceUtils.viewSource(SourceURL, null, this.iframeWindow.document,
        SourceLine || 0);
Comment on attachment 8783327 [details] [diff] [review]
hudservice_eslint_fix.patch

See Comment 8
Attachment #8783327 - Flags: feedback?(lclark)
Attachment #8783327 - Flags: feedback?(bgrinstead)
Attached patch hudservice_eslint_fix.patch (obsolete) — Splinter Review
I already had the patch without eslint errors with help of Nicolas Chevobbe.
Brian, please comment some docs link to learn to push to try after reviewing this. I tried to push to try but could not, I have level 1 access.
Thanks
Attachment #8783327 - Attachment is obsolete: true
Attachment #8784417 - Flags: review?(bgrinstead)
Comment on attachment 8784417 [details] [diff] [review]
hudservice_eslint_fix.patch

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

Please rebase this and make sure this file is included in the .eslintignore file with: !devtools/client/webconsole/hudservice.js
Attachment #8784417 - Flags: review?(bgrinstead)
Hi, sorry for being late. Hope this works :)
Attachment #8784417 - Attachment is obsolete: true
Attachment #8788191 - Flags: review?(bgrinstead)
Comment on attachment 8788191 [details] [diff] [review]
hudservice_eslint_fix.patch

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

There is at least one error coming through with this applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec93dd3e4c17&selectedJob=27540703 

TEST-UNEXPECTED-FAIL | devtools/client/scratchpad/test/browser_scratchpad_open_error_console.js | A promise chain failed to handle a rejection: - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/hudservice.js:658 - TypeError: this.target.client is null

Please check on any variable renames that all references to those variables are changed

::: devtools/client/webconsole/hudservice.js
@@ +616,1 @@
>      if (this._bc_init) {

All references of _bc_init should be replaced with bcInit

@@ +654,1 @@
>      if (this._bc_destroyer) {

All references of _bc_destroyer should be replaced with bcDestroyer
Attachment #8788191 - Flags: review?(bgrinstead) → review-
No longer blocks: 1256948
Going to handle this in Bug 1436076
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1436076
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.