Enable the semi ESLint rule across the tree

NEW
Assigned to

Status

Testing
Lint
P2
normal
6 months ago
14 days ago

People

(Reporter: standard8, Assigned: Dan Banner)

Tracking

Version 3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

6 months ago
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?
(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). :-)
(Reporter)

Comment 2

6 months ago
Yeah, we'll use --fix. We'll try and do this at a quiet time as well to avoid possible clashes.
I guess I slightly prefer turning that exception on, particularly if it applies to arrow functions, but I'm not tied to it.
(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. :-\
(Reporter)

Comment 5

6 months ago
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.
(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?
(Assignee)

Comment 7

5 months ago
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.
(Reporter)

Updated

3 months ago
Priority: -- → P2
(Reporter)

Comment 8

a month ago
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.
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
(Assignee)

Updated

a month ago
Depends on: 1408777
What's left to do here now that bug 1408777 is fixed? :-)
Flags: needinfo?(dbugs)
(Assignee)

Comment 11

14 days ago
update the rules and fixed anymore instancesand i will try to get it done by monday
Flags: needinfo?(dbugs)
You need to log in before you can comment on or make changes to this bug.