Closed Bug 1256773 Opened 4 years ago Closed 4 years ago

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


(DevTools :: Console, defect, P3)



(firefox48 fixed)

Firefox 48
Tracking Status
firefox48 --- fixed


(Reporter: linclark, Assigned: philr, Mentored)



(Whiteboard: [btpp-backlog])


(1 file)

If you haven’t contributed to Firefox before, follow the steps here to set up your environment:

Then, automatically configure ESLint to work with the Firefox specific rules by following the instructions here:

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

> eslint --no-ignore devtools/client/webconsole/panel.js
Blocks: 1256948
Priority: -- → P3
Whiteboard: [btpp-backlog]
Hi, I want to fix it. 
It's gonna be my first contribution and I'm just trying to figure out how to create patches and thing like this ;)

I've already fixed most of the problems but I'm not sure what to do with this methods:
focusInput: function WCP_focusInput()
open: function WCP_open()
destroy: function WCP_destroy()

I think there is a reason behind this naming, but I can't find any info about it.
Flags: needinfo?(lclark)

For patches, I use the git workflow described here:

As for the functions, there was a reason for that naming a while ago. We didn't used to have good stack traces, so those function names helped people figure out where errors were coming from. Our function naming has gotten better since then, so we don't need to have the function names anymore.
Flags: needinfo?(lclark)
Here is the patch, I hope it's in the right format =)
Flags: needinfo?(lclark)
Also I have a question: is it possible to address multiple bugs in one patch? that will allow to fix all eslint problems in the whole devtools/client/webconsole/ directory for example and not fix file by file.
Attachment #8738788 - Flags: review?(lclark)
Awesome, this looks good to me! I pushed it up to the test server, if that comes back green then this will be good to go:

If you want to take on a bigger chunk, how about the files in the test directory at devtools/client/webconsole/test? We can create a new bug for that and then close out the other ones when it lands.
Flags: needinfo?(lclark)
Looks like everything is green!

There are 800+ errors after running eslint with --fix option o_O It's gonna take some time ;)
Comment on attachment 8738788 [details] [diff] [review]
bug1256773_eslintfixes.patch is a patch with fixes

Review of attachment 8738788 [details] [diff] [review]:

Great! Feel free to add the keyword "checkin-needed" and someone will come along and merge it in.

If you want to do just a chunk of files at once, that works too. I'd like any of the files at the top level (not in the test directory) to be done 1-by-1 so we can give the review more attention, but combining issues in the test directory is fine.

Also, just FYI some of the issues in the test directory are with globals. Those globals can be set in a folder-wide config, so that might clear up a bunch of issues. If you want to add a patch with that, we'd be happy to accept it.
Attachment #8738788 - Flags: review?(lclark) → review+
I'm not assigned to the ticket ;) so I can't add keywords.
Thanks for the tips.
Flags: needinfo?(lclark)
ah, I didn't realize that! I've added the keyword and assigned to you
Assignee: nobody → philipp
Flags: needinfo?(lclark)
Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.