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)
Tree Management
Treeherder
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. :)
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
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)
Reporter | ||
Comment 3•8 years ago
|
||
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+
Assignee | ||
Comment 4•8 years ago
|
||
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?
Reporter | ||
Comment 5•8 years ago
|
||
(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.
Comment 6•8 years ago
|
||
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!
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8775028 -
Flags: feedback?
Comment 8•8 years ago
|
||
This looks good to me. Thanks for doing this! I'm going to try pushing this to stage to test it.
Comment 9•8 years ago
|
||
Comment on attachment 8775028 [details]
[treeherder] SJasoria:Bug_1289138 > mozilla:master
Great work!
Attachment #8775028 -
Flags: review?(cdawson) → review+
Comment 10•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 11•8 years ago
|
||
Comment 12•8 years ago
|
||
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.
Description
•