On 24/12/2017 19:41, Ben Kelly wrote:
But I also see rules about cosmetic things like what kind of quotes must be
used for strings.
AFAICT this kind of rule does not have any tangible safety benefit. Its
purely a cosmetic style choice. I don't see why we should bounce patches
out of the tree if the author and reviewer of a component prefer to use
single quotes instead of double quotes in a file.
As Jonathan already mentioned, the stylistic rules are designed to help
enforce a consistent style, and reduce code review cycles to address
review "nits" - benefiting both the submitter and reviewer.
This also helps newcomers find and pick up the consist style quickly,
rather than having to "examine the files in the component and work out
the style that's used most" which is what we've had in the past.
As a side note - the quotes rule was introduced at the request of
Firefox peers to have consistent use of quotes across style (a
consistent review nit, and frequently missed).
I also find this kind of cosmetic rule especially frustrating because we do
not run eslint by default. AFAIK it does not even install with mach
bootstrap. If we are going to enforce this kind of thing these checks
should be run as part of a build or when executing tests. They should not
require manual installation of tools and running separate commands.
Thank you for highlighting this. Last year we were working on improving
our hook support, with the intention of getting bootstrap hooked up as
well. We didn't get time for that, so that will be the first priority
this year.
I'm not yet convinced about hooking it into build / test execution. We
could do this, but there's a variety of issues about when we run them
and if we do/don't fail, that would make it harder in various instances
(e.g. we have a rule to disallow 'debugger' statements, but you might
want to run the tests with it). This might need a separate discussion.
As a further anecdote, I have also had to fight eslint rules in devtools
which required things like simultaneously using an extreme indent value and
not exceeding a line width limit. Basically conflicting and impossible
rules to satisfy. I ended up wasting close to an hour figuring out an ugly
work around for the problem. It did not feel like the lint was providing
value.
devtools started out with stricter rules overall. For ESLint for most of
mozilla-central, I think we have been conservative on the more stylistic
rules. Note, they can be disabled in various sections if it makes sense
(e.g. some of the accessible files disable no-multi-spaces, as it makes
sense for those code blocks, but not for other places in the tree). I
think longer term, we should consider changing the ESLint stylistic
rules to using something like the "prettier" tool across the tree
(similar to clang-format but for javascript - automatically formats).
Mark
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
