1508369 - Investigate adding stylelint (or some other css linter) to |mach lint|

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement, P3)

Description

[Andrew Halberstadt [:ahal]]

https://stylelint.io/

We don't have any css linters running in the tree right now, and this might be a good one to look into.

This runs with node, so I think we can re-use the package management :Standard8 set up for ESLint. It might even be as easy as adding the package to the top-level 'package.json' file.

Though :dmose notes that we have better node module integration coming, so it might also be worth waiting for that.

Comment 1

[Mark Banner (:standard8)]

I discussed this with Florian (I think) a while ago and the main conclusion was that we already have in tree tests that cover us for invalid css and unused variables. It also runs after any preprocessing so we don't have to do anything special for those files.

I think the first step would be figuring out what a linter would actually give us above the existing tests and then work out if it is worth it or not.

Comment 2

[Mike de Boer [:mikedeboer]]

I think the filing of this bug is inspired by https://groups.google.com/d/msg/firefox-dev/B7CFWHA_6BY/FvcfvD98AAAJ, which points to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/CSS_Guidelines.
This is a great new document that could serve as inspiration for 'enforcing' idiomatic style conventions using a linter.

Comment 3

[Andrew Halberstadt [:ahal]]

Yeah, should have mentioned that :)

I also want to mention that I personally don't have any stake in whether this gets implemented as I don't do frontend development. If the consensus is that this is worth doing, then I can help set it up. Otherwise we can WONTFIX.

For a bit of context, for each individual rule in the linter we can either:
a) Make it an 'error'  (i.e a violation would be backed out)
b) Make it a 'warning' (i.e it'll be hidden unless passing --warnings and won't cause a backout)
c) Ignore it outright

We can also run the linter only on specific subdirectories (though the preference of ESLint has been to make the config as global as possible, so we might want to follow suit).

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Definitely we could use help for styling nits. Linters are a great way to provide feedback to patch authors in a quick and less painful fashion. It can feel much worse when a person asks to fix minor nits versus an automated lint-bot on phabricator or a lint job that can be run locally. It's also faster for reviews in that the human reviewer doesn't need to comment about each little style issue.

We can start with a linter for style issues and then expand its powers as time goes on.

Comment 5

[:Gijs (out for now; he/him)]

Attachment: Bug 1508369 - clean up browser CSS to pass basic stylelint rules, r?#desktop-theme-reviewers

Depends on D177470

Comment 6

[:Gijs (out for now; he/him)]

Attachment: Bug 1508369 - add stylelint linter support to mach lint, r?Standard8!,dao!,#linter-reviewers!

Depends on D177476

Comment 7

[:Gijs (out for now; he/him)]

Attachment: Bug 1508369 - add stylelint job on taskcluster, r?Standard8!,#linter-reviewers

Depends on D177477

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b53dc7dcd3b0
clean up browser CSS to pass basic stylelint rules, r=desktop-theme-reviewers,webcompat-reviewers,extension-reviewers,devtools-reviewers,nchevobbe,denschub,dao

Comment 9

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/b53dc7dcd3b0

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7473b3223bd
add stylelint linter support to mach lint, r=Standard8,dao,linter-reviewers,sylvestre

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/a7473b3223bd

Comment 12

[:Gijs (out for now; he/him)]

Oops, forgot to drop the leave open.