MDL-52127 js: check amd files with eslint grunt task

I have spent quite a lot of time working through the current list of eslint options and configuring them for Moodle style and I think this is a very good basis to start us at (as well as taking some of out jshint options out with https://www.npmjs.com/package/polyjuice ). Thanks to Andrew Nicols, Mark Johnson and Frédéric Massart for some refinements. With this configuration the grunt build will fail if errors are present in the js (though you can of course tell jshint to ignore some errors, as I have done in admin/tool/lp/amd/src/competency_rule_points.js and defining the Y global in lib/amd/src/yui.js ). The grunt task will not report warnings by default, but a new --show-lint-warnings flag will help achieve that. Editor integrations/stanadalone eslint tool will surely be a better way of getting eslint errors rather than using the grunt task.
ralferlebach · Jun 11, 2016 · 3adb62b · 3adb62b
1 parent 834f1b6
commit 3adb62b
Show file tree

Hide file tree

Showing 8 changed files with 722 additions and 15 deletions.
diff --git a/.eslintignore b/.eslintignore
@@ -0,0 +1,3 @@
+theme/bootstrapbase/amd/src/bootstrap.js
+lib/amd/src/mustache.js
+lib/amd/src/loglevel.js
diff --git a/.eslintrc b/.eslintrc
@@ -0,0 +1,164 @@
+{
+  'env': {
+    'browser': true,
+    'amd': true
+  },
+  'globals': {
+    'M': true,
+    'Y': true
+  },
+  'rules': {
+    // See http://eslint.org/docs/rules/ for all rules and explanations of all
+    // rules. Commented out rules with 'DEFINE POLICY' are rules Dan P has flagged
+    // for discussion and possible enable soon.
+    // === Possible Errors ===
+    // DEFINE POLICY: 'comma-dangle': ['off', 'always'],
+    'no-cond-assign': 'error',
+    'no-console': 'error',
+    'no-constant-condition': 'error',
+    'no-control-regex': 'error',
+    'no-debugger': 'error',
+    'no-dupe-args': 'error',
+    'no-dupe-keys': 'error',
+    'no-duplicate-case': 'error',
+    'no-empty': 'error',
+    'no-empty-character-class': 'error',
+    'no-ex-assign': 'error',
+    'no-extra-boolean-cast': 'error',
+    'no-extra-parens': 'off',
+    'no-extra-semi': 'error',
+    'no-func-assign': 'error',
+    'no-inner-declarations': 'error',
+    'no-invalid-regexp': 'error',
+    'no-irregular-whitespace': 'error',
+    'no-negated-in-lhs': 'error',
+    'no-obj-calls': 'error',
+    'no-prototype-builtins': 'off',
+    'no-regex-spaces': 'error',
+    'no-sparse-arrays': 'error',
+    'no-unexpected-multiline': 'error',
+    'no-unreachable': 'warn',
+    'no-unsafe-finally': 'error',
+    'use-isnan': 'error',
+    'valid-jsdoc': ['warn', { 'requireReturn': false }],
+    'valid-typeof': 'error',
+
+    // === Best Practices ===
+    // (these mostly match our jshint config)
+    'curly': 'error',
+    'dot-notation': 'warn',
+    'no-alert': 'warn',
+    'no-caller': 'error',
+    'no-case-declarations': 'error',
+    'no-empty-pattern': 'error',
+    'no-empty-function': 'warn',
+    //DEFINE POLICY: 'no-eq-null': 'warn',
+    'no-eval': 'error',
+    //DEFINE POLICY: 'no-extra-bind': 'warn',
+    'no-fallthrough': 'error',
+    //DEFINE POLICY: 'no-implicit-globals': 'warn',
+    'no-implied-eval': 'error',
+    'no-invalid-this': 'error',
+    'no-iterator': 'error',
+    'no-labels': 'error',
+    'no-loop-func': 'error',
+    'no-multi-spaces': 'warn',
+    'no-multi-str': 'error',
+    'no-native-reassign': 'warn',
+    'no-new-func': 'error',
+    'no-new-wrappers': 'error',
+    // DEFINE POLICY: no-octal: "error"
+    // DEFINE POLICY: no-octal-escape: "error"
+    'no-proto': 'error',
+    'no-redeclare': 'warn',
+    'no-return-assign': 'error',
+    'no-script-url': 'error',
+    'no-self-assign': 'error',
+    'no-self-compare': 'error',
+    'no-unmodified-loop-condition': 'error',
+    'no-unused-expressions': 'error',
+    'no-unused-labels': 'error',
+    //DEFINE POLICY: 'no-useless-call': 'error',
+    'no-useless-escape': 'warn',
+    //DEFINE POLICY: 'no-with': 'error',
+    'wrap-iife': ['error', 'any'],
+
+    // === Variables ===
+    'no-delete-var': 'error',
+    'no-undef': 'error',
+    //DEFINE POLICY: 'no-undef-init': 'error',
+    'no-unused-vars': 'error',
+
+    // === Stylistic Issues ===
+    'array-bracket-spacing': 'warn',
+    'block-spacing': 'warn',
+    'brace-style': ['warn', '1tbs'],
+    'camelcase': 'warn',
+    'comma-spacing': ['warn', { 'before': false, 'after': true }],
+    'comma-style': ['warn', 'last'],
+    'computed-property-spacing': 'error',
+    'consistent-this': 'off',
+    'eol-last': 'off',
+    'func-names': 'off',
+    'func-style': 'off',
+    // indent currently not doing well with our wrapping style, so disabled.
+    'indent': ['off', 4, { 'SwitchCase': 1 }],
+    'key-spacing': ['warn', { 'beforeColon': false, 'afterColon': true, 'mode': minimum }],
+    'keyword-spacing': 'warn',
+    'linebreak-style': ['error', 'unix'],
+    'lines-around-comment': 'off',
+    'max-len': ['error', 132],
+    'max-lines': 'off',
+    // DEFINE POLICY: turn on some of these max values?
+    'max-depth': 'off',
+    'max-nested-callbacks': 'off',
+    'max-params': 'off',
+    'max-statements': 'off',
+    'max-statements-per-line': 'off',
+    'new-cap': 'warn',
+    'new-parens': 'warn',
+    'newline-after-var': 'off',
+    'newline-before-return': 'off',
+    // REVIST POLICY: 'newline-per-chained-call': 'warn',
+    'no-array-constructor': 'off',
+    'no-bitwise': 'error',
+    'no-continue': 'off',
+    'no-inline-comments': 'off',
+    'no-lonely-if': 'off',
+    'no-mixed-operators': 'off',
+    'no-mixed-spaces-and-tabs': 'error',
+    'no-multiple-empty-lines': 'warn',
+    'no-negated-condition': 'off',
+    'no-nested-ternary': 'warn',
+    'no-new-object': 'off',
+    'no-plusplus': 'off',
+    'no-spaced-func': 'warn',
+    'no-ternary': 'off',
+    'no-trailing-spaces': 'error',
+    'no-underscore-dangle': 'off',
+    // DEFINE POLICY: 'no-unneeded-ternary': 'off',
+    'no-whitespace-before-property': 'warn',
+    // DEFINE POLICY: 'object-curly-newline': 'off,
+    // DEFINE POLICY: 'object-curly-spacing': 'off',
+    // DEFINE POLICY: 'object-property-newline': 'off',
+    'one-var': 'off',
+    // DEFINE POLICY: 'one-var-declaration-per-line': 'off',
+    'operator-assignment': 'off',
+    'operator-linebreak': 'off',
+    'padded-blocks': 'off',
+    // DEFINE POLICY: 'quote-props': 'off',
+    'quotes': 'off',
+    'require-jsdoc': 'warn',
+    'semi': 'error',
+    'semi-spacing': ['warn', {'before': false, 'after': true}],
+    'sort-vars': 'off',
+    'space-before-blocks': 'warn',
+    'space-before-function-paren': ['warn', 'never'],
+    'space-in-parens': 'warn',
+    'space-infix-ops': 'warn',
+    'space-unary-ops': 'warn',
+    'spaced-comment': 'warn',
+    'unicode-bom': 'error',
+    'wrap-regex': 'off',
+  }
+}
diff --git a/Gruntfile.js b/Gruntfile.js
@@ -13,6 +13,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 /* jshint node: true, browser: false */
+/* eslint-env node */
 
 /**
  * @copyright  2014 Andrew Nicols
@@ -68,6 +69,15 @@ module.exports = function(grunt) {
             options: {jshintrc: '.jshintrc'},
             amd: { src: amdSrc }
         },
+        eslint: {
+            // Even though warnings dont stop the build we don't display warnings by default because:
+            // * At this moment we've got too many core warnings
+            // * It will complain about ignored files (https://github.com/sindresorhus/grunt-eslint/issues/119)
+            // * It's better experience to use editor integrations or eslint natively
+            options: { quiet: !grunt.option('show-lint-warnings') },
+            // Check AMD files with standard config
+            amd: { src: amdSrc },
+        },
         uglify: {
             amd: {
                 files: [{
@@ -238,10 +248,11 @@ module.exports = function(grunt) {
     grunt.loadNpmTasks('grunt-contrib-jshint');
     grunt.loadNpmTasks('grunt-contrib-less');
     grunt.loadNpmTasks('grunt-contrib-watch');
+    grunt.loadNpmTasks('grunt-eslint');
 
     // Register JS tasks.
     grunt.registerTask('shifter', 'Run Shifter against the current directory', tasks.shifter);
-    grunt.registerTask('amd', ['jshint', 'uglify']);
+    grunt.registerTask('amd', ['eslint:amd', 'jshint', 'uglify']);
     grunt.registerTask('js', ['amd', 'shifter']);
 
     // Register CSS taks.

diff --git a/admin/tool/lp/amd/src/competency_rule_points.js b/admin/tool/lp/amd/src/competency_rule_points.js
@@ -126,6 +126,7 @@ define(['jquery',
             try {
                 config = JSON.parse(self._competency.ruleconfig);
             } catch (e) {
+                // eslint-disable-line no-empty
             }
         }
 

diff --git a/lib/amd/src/localstorage.js b/lib/amd/src/localstorage.js
@@ -105,6 +105,7 @@ define(['core/config'], function(config) {
     var hashString = function(source) {
         // From http://stackoverflow.com/questions/7616461/generate-a-hash-from-string-in-javascript-jquery.
         /* jshint bitwise: false */
+        /* eslint no-bitwise: "off" */
         var hash = 0, i, chr, len;
         if (source.length === 0) {
             return hash;

diff --git a/lib/amd/src/yui.js b/lib/amd/src/yui.js
@@ -26,5 +26,6 @@
 define(function() {
 
     // This module exposes only the global yui instance.
+    /* global Y */
     return /** @alias module:core/yui */ Y;
 });