1596185 - [meta] Enable all rules for layout/

Status: NEW

(Core :: Layout, task)

Description

[Mark Banner (:standard8)]

When enabling ESLint on layout/, we disabled a lot of rules that were failing at the time.

We should work towards re-enabling those. At the moment, I think this would be better started after we enable prettier on html/xhtml files (bug 1560186), since that will give us automatic formatting for layout issues as a result of changes.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this about test files or non-test files?

I'm hesitant to apply things like this to test files because I think it's good that test files represent the diversity of coding styles that exist on the web (especially since many were simplified from real web pages). That makes it more likely that they'll catch regressions.

Comment 2

[Mark Banner (:standard8)]

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 from comment #1)

Is this about test files or non-test files?

Any non-test files (I think there's only one or two for layout), and any test file that isn't a crashtest nor a reftest. Note that things supplied from third-parties (e.g. web platform tests) are typically excluded as well, though I'm not sure layout/ has many of those.

I'm hesitant to apply things like this to test files because I think it's good that test files represent the diversity of coding styles that exist on the web (especially since many were simplified from real web pages). That makes it more likely that they'll catch regressions.

I understand the concerns, and it could go either way. Having ESLint enabled gives a better chance of finding errors in tests - and we have found a quite a few as we have gone through the rest of mozilla-central. It also helps provide us with a more consistent style for normal code as well as tests, which also reduces nits and turnaround cycles for reviews.

My general thought is that if we're writing a test a specific way for a specific reason that's not matching our styles, then we should be being explicit in calling that out. Otherwise, if someone comes along and cleans the test up later, we would loose that context. Obviously that's hard to apply retrospectively.

Though I also wonder how many of the rules would be likely to change a test in a way that would not be caught by e.g. the js engine tests, or something else of that level of testing.

Overall, I'm preferring if we can to get all rules applied everywhere with explicit call-outs in-line for disabling rules if necessary. That provides us with the greatest consistency for developers, as well as getting the protections against mistakes in tests.