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). :-)

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. :-\

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?

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.

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.

(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.

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

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

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.

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/40106dd2c532 Enable the semi ESLint rule across the tree. r=standard8

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

(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.

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

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.