Closed Bug 1508992 Opened 6 years ago Closed 6 years ago

Enable ESLint for dom/plugins

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: standard8, Assigned: championshuttler)

References

Details

Attachments

(4 files, 4 obsolete files)

Bug 1508992 - Enable ESLint for dom/plugins/test/mochitest (Automatic changes). r=standard8
(deleted), text/x-phabricator-request
Details
Bug 1508992 - Enable ESLint for dom/plugins/test/mochitest (Manual changes). r=standard8
(deleted), text/x-phabricator-request
Details
Bug 1508992 - Enable ESLint for dom/plugins/test/unit (Automatic Changes). r=standard8
(deleted), text/x-phabricator-request
Details
Bug 1508992 - Enable ESLint for dom/plugin/test/unit (Manual Changes). r=standard8
(deleted), text/x-phabricator-request
Details
As part of rolling out ESLint across the tree, we should enable it for dom/plugins.
Hi, I would like to take this issue :) Thank you!
Assignee: standard8 → huda_wadi
Hi Mark, I know I have realized this a little too late but I have noticed that in the .eslintignore file there is two directories that include Dom/plugins. Should I assume that I will be working on both? (I have already started on one of them but just want to confirm) thank you!
Hi hudaa, it would be nice to do both, but if you're pushed for time, just doing one is fine. If you've already done dom/plugins/test/mochitest then I think there's only a few things to fix for dom/plugins/test/unit/
I will definitely complete both :) I'm just struggling with a few errors that I can't seem to find any help with online. One of them is: 'Function testObject has a complexity of 35' ^ It's associated with a pretty long function(Please let me know if you would like me to post it here) and I can't seem to understand what I need to fix here for the error to go away. Another one Im having an issue with is: 'Expected to return a value at the end of arrow function' This is the code sample, I understand that it's expecting a return statement at the end but a lot of examples I see online talk about 'map' but I'm a bit confused as to what is expected to be return here 'SimpleTest.registerCleanupFunction(() => { if (this.document.mozFullScreenElement) { let fullScreenChange = promiseFullScreenChange(); this.document.mozCancelFullScreen(); return fullScreenChange; } });' Sorry for asking so many questions and I appreciate anything help I can receive. Thank you!
(In reply to hudaa from comment #4) > One of them is: > 'Function testObject has a complexity of 35' > ^ It's associated with a pretty long function(Please let me know if you > would like me to post it here) and I can't seem to understand what I need to > fix here for the error to go away. This is probably with the pre-existing functions, unfortunately some have got a bit too long and complex. In this case we tend to just disable the rule for when we're enabling ESlint on new code, so you can add a comment like: ``` // eslint-disable-next-line complexity function complexFunction() { ``` > Another one Im having an issue with is: > 'Expected to return a value at the end of arrow function' > > This is the code sample, I understand that it's expecting a return statement > at the end but a lot of examples I see online talk about 'map' but I'm a bit > confused as to what is expected to be return here > > 'SimpleTest.registerCleanupFunction(() => { > if (this.document.mozFullScreenElement) { > let fullScreenChange = promiseFullScreenChange(); > this.document.mozCancelFullScreen(); > return fullScreenChange; > } > });' So fullScreenChange is a Promise, and it is complaining that for the `!this.document.mozFullScreenElement` case that there's no return value. Probably the easiest thing is to add another line after the if section that is just `return Promise.resolve();`. The alternative would be to switch this to an async function and `await fullScreenChange` rather than return it, but I think that's a bit more than what we need here. > Sorry for asking so many questions and I appreciate anything help I can > receive. Thank you! No problem.
Thank you so much Mark!
Attachment #9029368 - Attachment description: Bug 1508992 - Enable ESLint for dom/plugins/test/mochitest/ (automatic changes) r?standard8! → Bug 1508992 - Enable ESLint for dom/plugins/test/mochitest/ (automatic changes)
Hi hudaa, I'm not quite sure what's happened here, but these are both automatic changes. Did you use moz-phab to submit them or arc directly? If you used arc, it might be best to swap to moz-phab as that handles multiple commits better. To do that, it is probably best to abandon these revisions (there's an action menu just above the comment box at the bottom), then, edit the two commits you've already made (with `hg histedit`) to remove anything apart from the descriptions. Then use moz-phab to submit.
Flags: needinfo?(huda_wadi)
I apologize for that, I have realized the mistake and I'm currently trying to fix them up. TO be honest, I'm still having a hard time getting moz-phab to setup and I'm currently doing some research to see what I'm doing wrong and why they are not going through. Will get this done asap.
Flags: needinfo?(huda_wadi)
Attachment #9029368 - Attachment is obsolete: true
Attachment #9029383 - Attachment is obsolete: true
Attachment #9029383 - Attachment is obsolete: false
Depends on D13719
Hi Mark, I have followed your suggestion and finally got moz-phab working (I think) I do see automatic and manual changes attachments now but not as comments above so I'm not sure if that's okay. Please let me know if something is wrong or if I need to redo something. Thank you for your patient!
Hi hudaa, https://phabricator.services.mozilla.com/D13769 seems wrong - it seems to be a very large diff that doesn't really relate to the proper item. Can you take another look. Using moz-phab you should just be able to have your two commits applied, then do `moz-phab submit` without any extra arguments.
Flags: needinfo?(huda_wadi)
Hi Mark, the problem Im having is, when I run 'moz-phab submit' I get a message that the command moz-phab is not found. I have a MacOS so according to the instruction here https://github.com/mozilla-conduit/review/blob/master/README.md I'm supposed to download the script and paste it in the path which I have done. I assumed we needed to run the scripts too but everytime I try to run it, I also get an error message that there's too few arguments (and the options i get is 'submit' which is supposed to commit to phabricator) and that's how I got the commits. I'm just confused as to how I should actually get moz-phab set up properly. Thank you!
Flags: needinfo?(huda_wadi)
Depends on D13769

Looks like its been a while for a update here. Can i work on it @Mark?

Most probably I will going to fix dom/plugins/test/mochitest/** then will attach patches for the next part :)

Thanks

Flags: needinfo?(standard8)

(In reply to Shivam Singhal [ :championshuttler ] from comment #16)

Looks like its been a while for a update here. Can i work on it @Mark?

In future I'd prefer a little more time between asking and working/attaching on patches. I don't work weekends, and some of these bugs I've been working with the people outside of bugzilla as it has been easier for discussion/sharing. I do generally keep track of these but I haven't been ready to resolve them all just yet.

Also where there's existing patches I'll sometimes prefer they are re-used.

In this case I haven't heard from hudaa for a while, so I assume they've gone onto other things, so I think we're ok to continue which you.

Assignee: huda_wadi → shivams2799
Flags: needinfo?(standard8)
Attachment #9029383 - Attachment is obsolete: true
Attachment #9029524 - Attachment is obsolete: true
Attachment #9030119 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [seneca-eslint] → checkin-needed

Now the directory dom/plugins/test/unit is remained, will attach the patches soon.

Removing checkin-needed, as try still seems unhappy :-( https://treeherder.mozilla.org/#/jobs?repo=try&revision=60cbeb71e2ed59a3b9ea252aba1f1e424c6daea8&selectedJob=230774572

Keywords: checkin-needed
Whiteboard: checkin-needed

I can't figure this out - it is working fine locally. So I'm doing a fresh try build to see if it was just a dodgy artifact build or something:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=584ffa0777adebb23c7000917565ac5b6ca46968

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8c1caae29d7c Enable ESLint for dom/plugins/test/mochitest (Automatic changes). r=Standard8,qdot https://hg.mozilla.org/integration/autoland/rev/2993263a193c Enable ESLint for dom/plugins/test/mochitest (Manual changes). r=Standard8,qdot
Status: NEW → ASSIGNED
Keywords: leave-open → checkin-needed
Whiteboard: checkin-needed

Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/519402d3b189
Enable ESLint for dom/plugins/test/unit (Automatic Changes). r=qdot
https://hg.mozilla.org/integration/autoland/rev/c7009174ac7e
Enable ESLint for dom/plugin/test/unit (Manual Changes). r=qdot

Keywords: checkin-needed
Whiteboard: checkin-needed

https://hg.mozilla.org/mozilla-central/rev/519402d3b189
https://hg.mozilla.org/mozilla-central/rev/c7009174ac7e

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: