Closed Bug 1508369 Opened 6 years ago Closed 11 months ago

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

Categories

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

enhancement

Tracking

(firefox115 fixed)

RESOLVED FIXED
Tracking Status
firefox115 --- fixed

People

(Reporter: ahal, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
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.
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.
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).
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.
Severity: normal → enhancement
Priority: -- → P3
See Also: → 1659444
See Also: 1561001
Severity: normal → --
See Also: → 1630027
Blocks: 1762027
See Also: prettier-css-format
See Also: 1659444
Product: Firefox Build System → Developer Infrastructure
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
See Also: → 1831299
See Also: → 1831300
Depends on: 1831302
Depends on: 1832029

Depends on D177477

See Also: → 1832101
Attachment #9332349 - Attachment description: Bug 1508369 - add stylelint job on treeherder, r?Standard8!,ahal! → Bug 1508369 - add stylelint job on treeherder, r?Standard8!,#linter-reviewers
Attachment #9332349 - Attachment description: Bug 1508369 - add stylelint job on treeherder, r?Standard8!,#linter-reviewers → Bug 1508369 - add stylelint job on taskcluster, r?Standard8!,#linter-reviewers
Keywords: leave-open
Pushed by gijskruitbosch@gmail.com:
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
See Also: → 1832130
Attachment #9332348 - Attachment description: Bug 1508369 - add stylelint linter support to mach lint, r?Standard8!,dao! → Bug 1508369 - add stylelint linter support to mach lint, r?Standard8!,dao!,#linter-reviewers
Attachment #9332349 - Attachment is obsolete: true
Attachment #9332348 - Attachment description: Bug 1508369 - add stylelint linter support to mach lint, r?Standard8!,dao!,#linter-reviewers → Bug 1508369 - add stylelint linter support to mach lint, r?Standard8!,dao!,#linter-reviewers!
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a7473b3223bd
add stylelint linter support to mach lint, r=Standard8,dao,linter-reviewers,sylvestre
Regressions: 1832661

Oops, forgot to drop the leave open.

Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1834590
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: