Closed
Bug 1158112
Opened 9 years ago
Closed 9 years ago
Move the Loop modules into a sub-directory and prepare eslint for enabling more rules for Loop
Categories
(Hello (Loop) :: Client, defect, P3)
Hello (Loop)
Client
Tracking
(firefox40 fixed)
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
References
Details
(Whiteboard: [tech-debt])
Attachments
(1 file, 1 obsolete file)
In preparing for bug 1150279, it would make a bit more sense from the eslint configuration perspective if our chrome module files were under a sub-directory. Then, the top-level configuration is what applies everywhere, with a few sub-directories turning on additional items (e.g. es6 rules). At the moment, we have the top-level configuration turning everything on, and disabling in sub-directories. I also want to move what we did in the etherpad into the configuration files, so that its easier for developers to know what we intend and we've got a record. Finally, whilst we're here, we should upgrade the eslint & eslint-plugin-react versions to the latest. There's a few new things that look useful and various bug fixes.
Assignee | ||
Comment 1•9 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #0) > In preparing for bug 1150279... I want to split that out into various good first bugs, hence doing some prep now seems worth it.
Assignee | ||
Comment 2•9 years ago
|
||
Somehow I thought I'd already attached this yesterday and obviously never completed it... This moves the files to a new modules directory and restructures the .eslintrc files. Now the "global" one is the minimum enabled checks for everything, and the sub-ones enable extra features (generally es6 things for chrome code). (Note: splinter review doesn't always show the moved files). I've also updated the eslint & eslint-plugin-react versions, there's various bug fixes and a few new features that we can pick up later. I've also pulled across the etherpad into the .eslintrc files so its clearer what we want as we start enabling the rules without needing to look in 2 places.
Attachment #8597605 -
Flags: review?(dmose)
Comment 3•9 years ago
|
||
Comment on attachment 8597605 [details] [diff] [review] Move the Loop modules into a sub-directory and prepare eslint for enabling more rules for Loop. Review of attachment 8597605 [details] [diff] [review]: ----------------------------------------------------------------- Looks good; this structure definitely feels more robust & cleaner. Some questions & comments interleaved. r=dmose ::: browser/components/loop/.eslintignore @@ +1,3 @@ > +# This file currently uses a non-standard (and not on a standards track) > +# if statement within catch. > +modules/MozLoopWorker.js Should we just stop doing that? Not necessarily here; could be in another bug... ::: browser/components/loop/.eslintrc @@ +31,5 @@ > // right now, we're bootstrapping the linting infrastructure. We'll > // want to audit these rules, and start turning them on and fixing the > // problems they find, one at a time. > > // Eslint built-in rules are documented at <http://eslint.org/docs/rules/> For the ones where we're not using the default, it'd be nice to have a brief comment explaining why we're doing something different, to help us if/when we decide to revisit particular issues in the future. @@ +32,5 @@ > // want to audit these rules, and start turning them on and fixing the > // problems they find, one at a time. > > // Eslint built-in rules are documented at <http://eslint.org/docs/rules/> > + "camelcase": 0, // Remove (use default) Maybe do a quick search/replace to say "TODO: Remove" or something like that? That'll make it more obvious to first-time code-readers who don't have the history here... @@ +73,5 @@ > // eslint-plugin-react rules. These are documented at > // <https://github.com/yannickcr/eslint-plugin-react#list-of-supported-rules> > "react/jsx-quotes": [2, "double", "avoid-escape"], > "react/jsx-no-undef": 2, > // Need to fix instances where this is failing. Is "this" supposed to be "these"? In either cases, maybe put a colon at the end of the line... since we're not visually delimiting with whitespace. ::: browser/components/loop/content/js/.eslintrc @@ +3,5 @@ > + // These are on for this directory for .jsm and content/js files. > + // If adding more items here, consider turning them off for the following > + // files if they aren't supported by all browsers. > + // content/shared/.eslintrc > + // content/standalone/.eslintrc Aren't lines 4-7 now obsolete and should be removed? ::: browser/components/loop/modules/.eslintrc @@ +3,5 @@ > + // These are on for this directory for .jsm and content/js files. > + // If adding more items here, consider turning them off for the following > + // files if they aren't supported by all browsers. > + // content/shared/.eslintrc > + // content/standalone/.eslintrc The previous four lines here want to go also, I think.
Attachment #8597605 -
Flags: review?(dmose) → review+
Assignee | ||
Comment 4•9 years ago
|
||
Updated patch for review comments. Requesting build config review.
Attachment #8597605 -
Attachment is obsolete: true
Attachment #8598492 -
Flags: review?(gps)
Updated•9 years ago
|
Rank: 30
Flags: firefox-backlog+
Whiteboard: [tech-debt]
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8598492 [details] [diff] [review] Move the Loop modules into a sub-directory and prepare eslint for enabling more rules for Loop. Expanding list of reviewers, as I'm looking to get this landed, as I've got some bugs waiting on this that I'm hoping new contributors will pick up soon.
Attachment #8598492 -
Flags: review?(ted)
Comment 6•9 years ago
|
||
Comment on attachment 8598492 [details] [diff] [review] Move the Loop modules into a sub-directory and prepare eslint for enabling more rules for Loop. Review of attachment 8598492 [details] [diff] [review]: ----------------------------------------------------------------- You don't need build peer review to reflect simple renames in moz.build files. And, I'm unqualified to review the eslint foo.
Attachment #8598492 -
Flags: review?(gps)
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8598492 [details] [diff] [review] Move the Loop modules into a sub-directory and prepare eslint for enabling more rules for Loop. Ah, I didn't realise that, thanks!
Attachment #8598492 -
Flags: review?(ted)
Assignee | ||
Updated•9 years ago
|
Iteration: --- → 40.3 - 11 May
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6eb3e36c9795
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•