Closed Bug 1289138 Opened 8 years ago Closed 8 years ago

Enable disabled eslint rules and fix corresponding js problems

Categories

(Tree Management :: Treeherder, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wlach, Assigned: ShrutiJ)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Working on bug 1289127, I realized there's a bunch of rules we ought to enable for eslint, but are currently disabled: https://github.com/mozilla/treeherder/blob/master/.eslintrc#L10 (you can just flip them from "0" to "2", the run "grunt checkjs" to find what fails, fix, and repeat) Shruti, do you want to try your hand at fixing these? Might be a good fast JavaScript learning experience between working on other bugs. :)
Blocks: 1183749
Comment on attachment 8775028 [details] [treeherder] SJasoria:Bug_1289138 > mozilla:master I am not able to fix 2 no-undef errors: https://github.com/mozilla/treeherder/blob/master/ui/js/filters.js#L20 Here platformNameMap throws no-undef, and https://github.com/mozilla/treeherder/blob/master/ui/js/perf.js#L92 here jobId throws no-undef How should I go about fixing these to errors? Other then these 2, I have tried to fix all of the errors. Please see and let me know if any change has to be made.
Attachment #8775028 - Flags: feedback?(wlachance)
Comment on attachment 8775028 [details] [treeherder] SJasoria:Bug_1289138 > mozilla:master A bunch of things don't seem quite right and this needs to be rebased against master, but looking good so far!
Attachment #8775028 - Flags: feedback?(wlachance) → feedback+
Comment on attachment 8775028 [details] [treeherder] SJasoria:Bug_1289138 > mozilla:master One more doubt: In https://github.com/SJasoria/treeherder/blob/f46edcfcef051f9c788cb75790e90f23d8dced1e/ui/js/controllers/jobs.js#L224 getBuildbotRequestId is giving no-undef error on running grunt checkjs. I found this function here: https://github.com/mozilla/treeherder/blob/master/ui/plugins/controller.js#L332 Is it a good idea to add 'ThJobArtifactModel' to jobs.js and then pull the data to get request_id the way it is done in controller.js? Other then this, I have addressed all the comments you made in PR :)
Attachment #8775028 - Flags: feedback+ → feedback?
(In reply to Shruti Jasoria [:ShrutiJ] from comment #4) > Comment on attachment 8775028 [details] > [treeherder] SJasoria:Bug_1289138 > mozilla:master > > One more doubt: > > In > https://github.com/SJasoria/treeherder/blob/ > f46edcfcef051f9c788cb75790e90f23d8dced1e/ui/js/controllers/jobs.js#L224 > > getBuildbotRequestId is giving no-undef error on running grunt checkjs. > > I found this function here: > https://github.com/mozilla/treeherder/blob/master/ui/plugins/controller. > js#L332 > > Is it a good idea to add 'ThJobArtifactModel' to jobs.js and then pull the > data to get request_id the way it is done in controller.js? Yes, you've found a bug! This code is currently broken. I believe your solution is correct too: we need to get the buildbot request id to send a manual cancellation request. This code is a bit tricky to test properly (you need credentials to do it), so I'll just do that myself tomorrow.
Depends on: 1290525
Shruti: this looks great to me. Just two small fixes in comments on the PR. Please let me know when you're done. Since a lot of this code will be affected by minimization, I'd like to test deploying to stage before we merge it. So I'll do that and then merge if all looks good. Thanks again!
Comment on attachment 8775028 [details] [treeherder] SJasoria:Bug_1289138 > mozilla:master I have made the changes which you had asked for in the PR. I hope its god now.
Attachment #8775028 - Flags: review?(cdawson)
Attachment #8775028 - Flags: feedback?
This looks good to me. Thanks for doing this! I'm going to try pushing this to stage to test it.
Comment on attachment 8775028 [details] [treeherder] SJasoria:Bug_1289138 > mozilla:master Great work!
Attachment #8775028 - Flags: review?(cdawson) → review+
Commit pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/4c8d3f69e67e5608c6241fe0bca69a78af150fe6 Bug 1289138 - Enable disabled eslint rules and fix corresponding js problems (#1736) Enable the following rules and fixed corresponding problems: * comma-dangle * no-console * no-redeclare * no-unused-vars * no-undef * no-unused-vars
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Commit pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/9b2bcda2c1da968255c599f32688e7ed7f10341d Bug 1289138 - Remove explicit enabling of rules that are enabled by default (#1785) Now that these rules are working, we don't need to explicitly enable them.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: