1561656 - Add WebExtensions directories to the .prettierignore file

Status: RESOLVED WONTFIX

(WebExtensions :: General, task)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

The WebExtensions internals use a set of eslint rules that is already enforcing coding style to be consistent, which is also stricter than the ruleset used in other parts of mozilla-central (and they differ from the ones currently enforced by the prettier config currently used on mozilla-central).

Comment 1

[Kris Maglione [:kmag]]

Note: We'll also need to disable the "prettier/prettier" rule in our base ESLint config file. With it enabled, ESLint adds way too much noise locally from the "prettier" rule to be remotely useful.

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1561656 - Add WebExtensions directories to the .prettierignore file. r?kmag!,vporof!

Comment 3

[Victor Porof [:vporof][:vp]]

We understand that coding styles can be subjective, and that this can be an invasive change.

The main purpose of using Prettier across mozilla-central is having more consistency and predictability, not just within teams but within Mozilla. We still have styling inconsistencies in our codebase and we have been unnecessarily spending human time when writing, reviewing, and discussing code. The prime directive is therefore to have consistent, predictable and enforceable styling.

Starting with July 5, Mozilla will not continue using eslint for enforcing styling. Rules strictly pertaining to code quality will be preserved, and this task will continue to be in eslint’s domain. Most styling-related rules will be removed in favour of separating this concern into Prettier’s domain. Some of the lines may be blurry here, however Prettier style has precedence and conflicting or redundant eslint rules will be removed.

I elaborate more on why in the email sent to dev-platform and firefox-dev two weeks ago, and I hope that the presentation at the all-hands in Whistler shed additional light on this and how we picked both Prettier and the configuration rules. There's also a quick FAQ in https://docs.google.com/document/d/1UDERMflocqdszMGhhtxiVhaCTVOHo6jxLsGbt4BR9uw/

When the changes were approved both by Firefox senior engineering leadership and the Firefox Technical Leadership Module, we’ve decided that compatibility with the existing styling is not an end-goal, and eslint strictness is not favoured over Prettier.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

is there any chance that we could be allowed to have an additional .prettierrc file in our directories?
(I briefly checked and a prettierrc included at the directory level seems to be used and behave as expected when mach lint --fix is reformatting files at that level).

That would allow us at least to tweak a couple of the few configuration options provided by prettier, so that the style enforced automatically by prettier from now on can still be a bit more similar to what we would have preferred.

Prettier doesn't support many configuration but the following one may still be nice for us: bracketSpacing: false.

Comment 5

[Victor Porof [:vporof][:vp]]

(In reply to Luca Greco [:rpl] from comment #4)

is there any chance that we could be allowed to have an additional .prettierrc file in our directories?
(I briefly checked and a prettierrc included at the directory level seems to be used and behave as expected when mach lint --fix is reformatting files at that level).

That would allow us at least to tweak a couple of the few configuration options provided by prettier, so that the style enforced automatically by prettier from now on can still be a bit more similar to what we would have preferred.

Prettier doesn't support many configuration but the following one may still be nice for us: bracketSpacing: false.

Iff there is a technical reason for using different bracket spacing or styling in general, pieces of source can be ignored using // prettier-ignore, or adding the respective file to .prettierignore top-level. This is for example necessary for various SpiderMonkey tests which verify actual syntax and the parser working, or for certain debugger test fixtures.

Iff the requirements for different styling are not technical, having additional .prettierrc further down the tree is not planned.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Victor Porof [:vporof][:vp] from comment #5)

Iff there is a technical reason for using different bracket spacing or styling in general, pieces of source can be ignored using // prettier-ignore, or adding the respective file to .prettierignore top-level. This is for example necessary for various SpiderMonkey tests which verify actual syntax and the parser working, or for certain debugger test fixtures.

Iff the requirements for different styling are not technical, having additional .prettierrc further down the tree is not planned.

Hi Victor,
thanks for evaluating our request.

Based on the above comment it seems that we don't have the kind of reasons that would allow an additional .prettierrc file or additional exclusions to the .prettierignore file. There may be an argument about skipping some files in toolkit/mozapps/extensions that should be removed soon as part of the migration from the XUL about:addons to the HTML about:addons page, but it is still likely not "a good enough reason" to exclude those.

I'm adding a needinfo assigned to Kris, in case he have some additional reasons that can justify the exclusion of some specific files, but in the meantime I'm abandoning the phabricator revision and closing this issue as wontfix (Kris can re-open it if we have specific reasons to exclude some specific file).

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Cleared an old needinfo.