mozillascience / PaperBadger

Issuing badges to credit authors for their work on academic papers
https://badges.mozillascience.org/
Mozilla Public License 2.0
95 stars 45 forks source link

Update linting to use mofo-style #166

Closed abbycabs closed 8 years ago

abbycabs commented 8 years ago

Mozilla Foundation JS Style Guide (mofo-style): https://github.com/MozillaFoundation/mofo-style

The Mozilla Foundation JS Style Guide has migrated from jshint & jscs to ESLint. Update this repo for consistency between foundation projects.

josmas commented 8 years ago

@acabunoc, I have upgraded to the latest mofo-style, and I get the scary number of ✖ 438 problems (396 errors, 42 warnings).

I can auto --fix the first pass, and the number goes down to ✖ 194 problems (152 errors, 42 warnings) which is a bit more manageable. But just wondering: the new mofo-style seems to be geared to fully ES6 projects, and I am not sure this project fully uses ES6. For instance, 100+ errors come from using quotes instead of backticks in strings; see sample diff from autofix:

--- a/src/app.js
+++ b/src/app.js
@@ -1,30 +1,30 @@
-var bodyParser = require('body-parser');
-var express = require('express');
-var path = require('path');
+var bodyParser = require(`body-parser`);
+var express = require(`express`);
+var path = require(`path`);

Many of the issues also come from using functions before defining them (this is something I modified in #165 to make jshint happy too).

So the main question is: can the rules in .eslintrc.yaml be relaxed slightly for this particular project? I can make the changes either way, but it will take a bit longer to fix all of 438 issues (I have no problems with that), and it will create a rather beefy pull request for review (with so many changes). Happy to go either way.

josmas commented 8 years ago

In fact, in order to allow for changes like the use of backtick, babel (or similar) will need to be hooked into the build process. Right now babel is used for react, but webpack will need an extra loader for ES6 transforms.

abbycabs commented 8 years ago

Whoa, yes, this is a heavy lift @josmas! Didn't expect you to jump on this so soon :)

I hadn't looked too deeply into the new mofo-style -- thank you for pointing out the 'fully ES6' direction!

In that case, let's use a relaxed version of .eslintrc.yaml (happy to let you decide what that means). Then, when/if we ever fully migrate to ES6, it'll be easier to migrate to the official mofo-style version of .eslintrc.yaml.

Thanks so much for your research here @josmas!

josmas commented 8 years ago

@acabunoc #168 uses a modified .eslintrc.yaml as a first step towards using the latest version of mofo-style. More details in the pull request. Let me know what you think.

abbycabs commented 8 years ago

You are a HERO for going through all that! I'll take a look tomorrow, just wanted to drop a quick thanks

On Mar 7, 2016, at 6:39 PM, Jos notifications@github.com wrote:

@acabunoc #168 uses a modified .eslintrc.yaml as a first step towards using the latest version of mofo-style. More details in the pull request. Let me know what you think.

— Reply to this email directly or view it on GitHub.

josmas commented 8 years ago

No probs! It's a big one, no rush; whenever you can find time.