[eslint] Turn on JavaScript linting and formatting rules for .sjs files
Categories
(Developer Infrastructure :: Lint and Formatting, task, P3)
Tracking
(firefox95 fixed)
Tracking | Status | |
---|---|---|
firefox95 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: janey, Mentored)
References
Details
(Whiteboard: [lang=js][good-next-bug])
Attachments
(2 files)
*.sjs
files are commonly used to write tests in the tree. It would be nice to have JS linting and prettier formatting for them.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 1•3 years ago
|
||
I'm happy to mentor this. Here's a summary of what needs doing:
- Create a commit that turns off prettier for the "tables" of arrays in the following files:
toolkit/components/passwordmgr/test/authenticate.sjs
layout/style/test/redundant_font_download.sjs
dom/serviceworkers/test/fetch/deliver-gzip.sjs
browser/components/urlbar/tests/browser/authenticate.sjs
browser/components/extensions/test/browser/authenticate.sjs
- Add
sjs
to the list of extensions intools/lint/eslint.yml
- Run
./mach eslint --fix
(this may take a while). - From the output of the above command, there will be various rules that fail. Update the top-level
.eslintrc.js
to add a section intooverride
to set those rules as warnings for*.sjs
files (e.g. similar to this example but withwarn
rather thanoff
) - Run
./mach eslint
again to ensure it now passes. - Create a commit of everything excluding the
.eslintrc.js
andtools/lint/eslint.yml
changes. When creating that commit add# ignore-this-changeset
onto the second or third line of the commit message. - Create a commit of just the
.eslintrc.js
andtools/lint/eslint.yml
changes.
Submit those two commits using moz-phab - they should create two revisions that are attached to this bug.
Assignee | ||
Comment 2•3 years ago
|
||
Hey It's Jane here - I am an Outreachy applicant and would love to work on this one please!Hey It's Jane here - I am an Outreachy applicant and would love to work on this one please!
Comment 3•3 years ago
|
||
Hi Jane, as this one is a little bit bigger, I'll assign it to you direct. Please do start working on them though if no-one else has commented already.
Assignee | ||
Comment 4•3 years ago
|
||
Hey Mark! Sounds great! A bit of clarification - when we turning off prettier for tables of arrays do we do same named specifically for table or for the whole files? Like:
ChromeUtils.defineModuleGetter(this, "toBinaryTable", "resource:///browser/components/extensions/test/browser/authenticate.sjs");
Haven't reach step 4 yet, probably will have some q there.
(In reply to Mark Banner (:standard8) from comment #3)
Assignee | ||
Comment 5•3 years ago
|
||
When I run mach lint straight up gives me issue -
55:1 error 'toBinaryTable' is defined but never used. no-unused-vars (eslint)
Is it even matter?
When I try a whole name one -
ChromeUtils.defineModuleGetter( this, "authenticate", "resource:///browser/components/extensions/test/browser/authenticate.sjs");
lint still gives same one:
55:1 error 'authenticate' is defined but never used.
thanks!
Comment 6•3 years ago
|
||
(In reply to Evgenia Kotovich from comment #4)
Hey Mark! Sounds great! A bit of clarification - when we turning off prettier for tables of arrays do we do same named specifically for table or for the whole files? Like:
Sorry, I wasn't quite clear enough there. The link was pointing to an example for how to turn prettier off then on again - these are the two highlighted lines.
In the case of toolkit/components/passwordmgr/test/authenticate.sjs
we have code laid out a bit like a table, and so we want to disable prettier there, and we need to add the two lines to the existing code, so you'd end up with something like:
// base64 decoder
//
// Yoinked from extensions/xml-rpc/src/nsXmlRpcClient.js because btoa()
// doesn't seem to exist. :-(
/* Convert Base64 data to a string */
/* eslint-disable prettier/prettier */
const toBinaryTable = [
-1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1,
-1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1,
-1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,62, -1,-1,-1,63,
52,53,54,55, 56,57,58,59, 60,61,-1,-1, -1, 0,-1,-1,
-1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9,10, 11,12,13,14,
15,16,17,18, 19,20,21,22, 23,24,25,-1, -1,-1,-1,-1,
-1,26,27,28, 29,30,31,32, 33,34,35,36, 37,38,39,40,
41,42,43,44, 45,46,47,48, 49,50,51,-1, -1,-1,-1,-1
];
/* eslint-enable prettier/prettier */
const base64Pad = '=';
So in each file, you'll need to find the bit of code that looks a bit like a table, and add the comments to surround it.
Assignee | ||
Comment 7•3 years ago
|
||
ignore-this-changeset
Assignee | ||
Comment 8•3 years ago
|
||
Depends on D128482
Updated•3 years ago
|
Comment 9•3 years ago
|
||
Adding leave-open as we're going to land the automatic changes while we're still working on the second part.
Updated•3 years ago
|
Comment 10•3 years ago
|
||
Comment 11•3 years ago
|
||
Backed out changeset 7c08aa027893 (Bug 1576768) for causing multiple failures.
Backout link: https://hg.mozilla.org/integration/autoland/rev/17cf6105da958084f8a75f4941a3135f4a274cf8
Push with failures.
Failure logs:
Comment 12•3 years ago
|
||
I took a look and it seems to be that the automated changes use ChromeUtils
which isn't currently available to sjs files. As far as I know, there's no reason it can't be made available, so I've filed bug 1736058 to add it.
I'm currently running it on try server run with both patches to make sure there's no other issues:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f46e7788599e6199ef30077c2944d90cd7f5e93
Updated•3 years ago
|
Comment 13•3 years ago
|
||
After the try server run in the previous comment, I noticed that some of the automatic rewrites from Components.utils
-> ChromeUtils
were wrong for the import cases, as they weren't specifying the global to be imported into.
As this was an issue with the automatic rewrite, I took the liberty of fixing those and I'm pushing an update at the moment. I also fixed a few more cases where there were tables that we don't want formatting - I'd missed those when originally writing comment 0.
https://treeherder.mozilla.org/jobs?repo=try&revision=b54021bfd4607569d38f3982cc6aa5fb343c57ba
Comment 14•3 years ago
|
||
Comment 15•3 years ago
|
||
Backed out changeset 2ab6bb03dcc1 (Bug 1576768) for causing failures in test_double_submit.html CLOSED TREE
Log: https://treeherder.mozilla.org/logviewer?job_id=355108860&repo=autoland&lineNumber=2808
Backout: https://hg.mozilla.org/integration/autoland/rev/8ebb25b5259154905d5971154709c2aff6346437
Comment 16•3 years ago
|
||
Ok, for some reason that didn't want to use ChromeUtils on Android - I've reverted that part of the change for now, and will file a follow-up later.
Comment 17•3 years ago
|
||
Comment 18•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Comment 19•3 years ago
|
||
Comment 20•3 years ago
|
||
Backed out for causing mochitest failures. CLOSED TREE
Backout link : https://hg.mozilla.org/integration/autoland/rev/0623bc5b7d4a40555459764fc4caf82b5badf689
Link to failure log :
https://treeherder.mozilla.org/logviewer?job_id=355740091&repo=autoland&lineNumber=3225
https://treeherder.mozilla.org/logviewer?job_id=355740817&repo=autoland&lineNumber=3429
https://treeherder.mozilla.org/logviewer?job_id=355740093&repo=autoland&lineNumber=2465
Comment 21•3 years ago
|
||
The issues have been fixed, and we're going to re-land.
Comment 22•3 years ago
|
||
Comment 23•3 years ago
|
||
bugherder |
Updated•2 years ago
|
Description
•