Proposal: Require dangling commas for multiline objects/arrays on all Javascript mozilla-central code

[Mark Banner]

I would like to propose that we require dangling commas for multi-line 
object/arrays in Javascript code for mozilla-central.


Why:

 * Multiple components in toolkit/ and browser/ already have
   comma-dangle enabled
   
.
 * Having asked around in a few locations, I'm seeing support from the
   Firefox developer team and others in favour of enabling it.
 * It helps make blame cleaner.
 * It makes editing easier, and helps for a consistent style.
 * Having automation available to cover the requirement reduces the
   need for review nits.

I realise not everyone necessarily loves the style, but having the 
cleaner blame, and consistent style requirements seems to outweigh the 
downsides.


How:

 * We would enable this via an ESLint rule, comma-dangle
   , with the
   "always-multiline" option set.
 * ESLint is able to automatically fix code that doesn't conform (e.g.
   bug 1476228 
   was entirely automatically generated very quickly)

Since there are a large amount of instances (approx 12k) that would need 
to be fixed across the code base, I would propose that we roll it out on 
a per-directory basis. For example, we could do all of browser/ at the 
start of the 64 cycle just after the merges (alternately the end of the 
63 cycle during soft freeze), then do toolkit/ at the next merge time, 
and then pick up the rest at the end. This will limit the scope of 
possible bitrotting to smaller chunks, as well as making the patch sizes 
more manageable.


This will only cover directories that ESLint currently covers - though 
as we roll out ESLint to new directories, they'll be covered automatically.


Feedback & thoughts welcome.

Standard8

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Proposal: Require dangling commas for multiline objects/arrays on all Javascript mozilla-central code

[Kris Maglione]

On Wed, Aug 22, 2018 at 09:29:26PM +0100, Mark Banner wrote:
I would like to propose that we require dangling commas for multi-line 
object/arrays in Javascript code for mozilla-central.


+1

I've lost count of the number of times I've hit an obscure 
failure because I added a new element to the end of an array or 
object without realizing the last element didn't have a comma. 
Most of the areas I'm responsible for already require this rule, 
and it's saved me a lot of headaches.


--
Kris Maglione

The use of COBOL cripples the mind; its teaching should therefore be
regarded as a criminal offense.
--Edsger W. Dijkstra

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Proposal: Require dangling commas for multiline objects/arrays on all Javascript mozilla-central code

[Gijs Kruitbosch]

On 22/08/2018 21:29, Mark Banner wrote:
I would like to propose that we require dangling commas for multi-line 
object/arrays in Javascript code for mozilla-central.


Strong +1 from me as well.

I would propose that we roll it out on 
a per-directory basis. For example, we could do all of browser/ at the 
start of the 64 cycle just after the merges (alternately the end of the 
63 cycle during soft freeze),


Out of these I'd prefer end-of-soft-freeze to make uplifts easier 
(because fewer conflicts).


Thanks for doing this!

Gijs
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Proposal: Require dangling commas for multiline objects/arrays on all Javascript mozilla-central code

[Mark Banner]

On 22/08/2018 21:29, Mark Banner wrote:


Since there are a large amount of instances (approx 12k) that would 
need to be fixed across the code base, I would propose that we roll it 
out on a per-directory basis. For example, we could do all of browser/ 
at the start of the 64 cycle just after the merges (alternately the 
end of the 63 cycle during soft freeze), then do toolkit/ at the next 
merge time, and then pick up the rest at the end. This will limit the 
scope of possible bitrotting to smaller chunks, as well as making the 
patch sizes more manageable.


I've not heard any negative comments, so I am moving forward with this. 
Having looked at the stats a bit more, I'm going to land this in two 
chunks. The first will be to enable the rule on files in these 
sub-directories of mozilla-central:


browser/ services/ toolkit/ taskcluster/

I'm aiming to land these on Friday (31st August), ahead of the 63 merges 
to beta. As others suggested, this should make uplifts easier. Bug 
1486739  is 
tracking this.


For the rest of the files, I'm currently looking at Friday 19 October, 
ahead of the 64 merges to beta. Bug 1486741 
 will track this part.


Mark.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform