1367704 - Enable the semi ESLint rule across the tree

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

I keep finding locations where they are missing, and seeing as we are generally consistent in requiring semicolons, I think we should just get this rule enabled.

Additionally, the rule is already enabled for some parts of the tree.

http://eslint.org/docs/rules/semi

There's one option available:

    "omitLastInOneLineBlock": true

I'm not sure if we should enable that or not. Out of the existing areas with semi enabled, only browser/components/migration has this enabled:

https://dxr.mozilla.org/mozilla-central/search?q=regexp%3A%5C%22semi%5C%22+file%3A.eslintrc.js&redirect=false

Mossop (or anyone else), thoughts?

Comment 1

[:Gijs (he/him)]

(In reply to Mark Banner (:standard8) from comment #0)
> There's one option available:
> 
>     "omitLastInOneLineBlock": true
> 
> I'm not sure if we should enable that or not. Out of the existing areas with
> semi enabled, only browser/components/migration has this enabled:
> 
> https://dxr.mozilla.org/mozilla-central/
> search?q=regexp%3A%5C%22semi%5C%22+file%3A.eslintrc.js&redirect=false
> 
> Mossop (or anyone else), thoughts?

I don't feel strongly - I used it for migration because not using it seemed to add needless churn to convert things like:

foo() { return bar }

into

foo() { return bar; }

which gets even worse (and slightly ugly, imho) if it's inline in a parameter:

foo.bar(function() { return baz[;] });

but really, I presume we can use --fix with this and just move on, if that's what we want to do, for either convention, and I'd be happy with that (either way around). :-)

Comment 2

[Mark Banner (:standard8)]

Yeah, we'll use --fix. We'll try and do this at a quiet time as well to avoid possible clashes.

Comment 3

[Dave Townsend [:mossop]]

I guess I slightly prefer turning that exception on, particularly if it applies to arrow functions, but I'm not tied to it.

Comment 4

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] from comment #3)
> I guess I slightly prefer turning that exception on, particularly if it
> applies to arrow functions, but I'm not tied to it.

Yeah... tbh, I think what I'd prefer most of all is allowing both forms (() => { return foo } as well as () => { return foo; }) in that particular case, but I don't think eslint lets us do that. :-\

Comment 5

[Mark Banner (:standard8)]

Just checked ESLint doesn't allow it to be optional, which is consistent with the intent of providing consistent styling.

I think given where we are, lets turn the exception on, and see how it goes.

Comment 6

[Florian Quèze [:florian]]

(In reply to Mark Banner (:standard8) from comment #5)
> Just checked ESLint doesn't allow it to be optional

That's unfortunate :-(.
When writing new code like "foo() { return bar; }" I would always include the semi colon.

Can we check how many lines would need to change in our tree with and without the option, and decide to go with what requires the smallest amount of changes to existing code?

Comment 7

[Dan Banner]

From the latest master:

With omitLastInOneLineBlock = false (the default)
-> 3234 issues

With omitLastInOneLineBlock = true
-> 4285 issues

From other measurements, we think there's about 1600 cases which are affected by omitLastInOneLineBlock. The rest of the issues are "normal" semi-colons on end of lines.

Comment 8

[Mark Banner (:standard8)]

In https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods the only reference I can see to semicolons, there's an example with the semi-colon in the block.

So I think it is time to make a call on this. I think based on the stats, we should probably leave the option off (i.e. require a colon). We'll at least be consistent throughout the tree.

Dave, are you in agreement with that? The only other alternative to get to a solution I can think of would be to discuss on a mailing list, but I'm not convinced that would get there.

Comment 9

[Dave Townsend [:mossop]]

(In reply to Mark Banner (:standard8) from comment #8)
> In
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#Methods the only reference I can see to semicolons, there's an
> example with the semi-colon in the block.
> 
> So I think it is time to make a call on this. I think based on the stats, we
> should probably leave the option off (i.e. require a colon). We'll at least
> be consistent throughout the tree.
> 
> Dave, are you in agreement with that? The only other alternative to get to a
> solution I can think of would be to discuss on a mailing list, but I'm not
> convinced that would get there.

I agree.

Comment 10

[:Gijs (he/him)]

What's left to do here now that bug 1408777 is fixed? :-)

Comment 11

[Dan Banner]

update the rules and fixed anymore instancesand i will try to get it done by monday

Comment 12

[Dan Banner]

Attachment: Bug 1367704 - Enable the semi ESLint rule across the tree.

Review commit: https://reviewboard.mozilla.org/r/201058/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201058/

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8929878 [details]
Bug 1367704 - Enable the semi ESLint rule across the tree.

https://reviewboard.mozilla.org/r/201058/#review206218

Looks good. r=Standard8 if the try server build passes, and with the two issues mentioned below fixed.

::: commit-message-b0565:1
(Diff revision 1)
> +Bug 1367704 - Enable the semi ESLint rule across the tree. r?Standard8

Please can you also bump the version number in tools/lint/eslint/eslint-plugin-mozilla/package.json and package-lock.json (same dir).

::: npm-shrinkwrap.json:1439
(Diff revision 1)
>      "sprintf-js": {
>        "version": "1.0.3",
>        "resolved": "https://registry.npmjs.org/sprintf-js/-/sprintf-js-1.0.3.tgz",
>        "integrity": "sha1-BOaSb2YolTVPPdAVIDYzuFcpfiw="
>      },
> +    "string_decoder": {

Please undo the change to this file.

Comment 14

[Dan Banner]

Comment on attachment 8929878 [details]
Bug 1367704 - Enable the semi ESLint rule across the tree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201058/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40106dd2c532
Enable the semi ESLint rule across the tree. r=standard8

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/40106dd2c532

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This caused issues in bug 1421048 because the changes to pdf.js weren't upstreamed :(. Please be more careful in the future.

Comment 18

[Dave Townsend [:mossop]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> This caused issues in bug 1421048 because the changes to pdf.js weren't
> upstreamed :(. Please be more careful in the future.

I don't know that we have a policy requiring folks landing in m-c to also upstream changes. Certainly it was up to the add-on sdk team to uplift such changes and I've always made it clear to any others wanting to live outside the tree that it would be up to them too.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Even a simple heads-up would have been nice rather than my first hearing of it being when ESLint went orange on inbound.

Comment 20

[Mark Banner (:standard8)]

Dan wouldn't have know about this, and most new contributors wouldn't. If anything it was me not raising it, however as Dave says, my general understanding is that people who are working outside of m-c are expected to keep an eye out for changes landing in m-c affecting their code. I'd also expect them to be running a few tests before merging (you have the hooks set-up right?).

Whilst I do try and remember to keep some of these external ones up to date, I've just noticed that pdf.js isn't using the mozilla:recommended configuration which also won't help, so I think I'll also go file an issue on them to possibly switch to it.