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)

defect
Points:
2

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Iteration:
40.3 - 11 May
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.
(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.
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 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+
Updated patch for review comments. Requesting build config review.
Attachment #8597605 - Attachment is obsolete: true
Attachment #8598492 - Flags: review?(gps)
Rank: 30
Flags: firefox-backlog+
Whiteboard: [tech-debt]
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 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)
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)
Iteration: --- → 40.3 - 11 May
https://hg.mozilla.org/mozilla-central/rev/6eb3e36c9795
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.