Updated style guide for JavaScript

[tyearke]

Description

This pull request updates the style guide for JavaScript code and enforces the changes with ESLint.

The commits in this pull request:

1. Update ESLint to a newer version compatible with the latest version of the Airbnb ES5 rules.
2. Modify the ESLint config file to use strings instead of integers for clarity (e.g. "error" instead of 2).
3. Apply the Airbnb ES5 rules. This commit also removes redundant custom rules and ensures that conflicting custom rules are still applied. (See this for more information on how rule overrides work in ESLint.)
4. Tweaked custom rules further based on team preferences.

Motivation and Context

Our current style guide is a good start, but it leaves room for a number of inconsistencies. When ESLint's auto-fixer is run, it can produce results that don't mesh with our implicit, existing style, like the example below.

// current
if (config) jQuery.extend(true, conf, config);

// ESLint auto-fix result
if (config) {jQuery.extend(true, conf, config)};

// desired result
if (config) {
    jQuery.extend(true, conf, config);
}

This pull request aims to tighten up the rules by using a more thorough style guide as a basis for ours. Since there is no official or semi-official style guide for JavaScript along the lines of PHP's PSR-2 or Python's PEP 8, a popular third-party guide was chosen - namely, the ES5 variant of Airbnb's guide.

I've made a handful of tweaks to their guide already based on discussions with various team members, but our style guide should be something the whole team feels comfortable with. If there are any rules you would like to add, modify, or delete, please comment. (I recommend running ESLint on some sample files with the new ruleset to see what it does and doesn't flag.)

The overrides of Airbnb's rules, both preexisting and new, are listed in the table below. If an option for a custom rule in the .eslintrc.json file is not described below, it is likely because it was copied from the Airbnb config due to how ESLint handles rule overrides.

Airbnb Rule	Custom Rule	Explanation	ESLint Setting
Allow brace-less single-line control statement bodies.	Require braces for all code blocks.	A consistent structure for code blocks makes it easier and less error prone to extend a one-line block.	curly, 1st option
Allow opening and closing braces of code blocks to be on same line.	Require opening and closing braces of code blocks to be on different lines.	A consistent structure for code blocks makes it easier and less error prone to extend a one-line block.	brace-style, 2nd option, allowSingleLine
Allow all implicit type conversions.	Disallow implicit conversions to numbers and booleans.	Implicit conversions can make code harder to follow than when explicit conversions are used.	no-implicit-coercion
Allow assignment of this to any variable.	Require this to be assigned to variables named self.	Consistent variable names for capturing execution context makes code easier to follow.	consistent-this
Indent using 2 spaces.	Indent using 4 spaces.	Most code in Open XDMoD, JavaScript or otherwise, uses 4 spaces already.	indent, 1st option
Require function expressions to have a name.	Allow function expressions to be anonymous.	Naming functions assigned to variables can be handy for debugging, but it is not worth throwing errors over.	func-names
Require variables to be declared at the top of their scope.	Don't require variables to be declared at the top of their scope.	Variables should be declared near where they are first used for readability.	vars-on-top
Require constructor functions start with a capital letter when the function is a property of an object.	Don't require constructor functions start with a capital letter when the function is a property of an object.	Ext JS stores some constructor functions in objects using camel-case names (e.g., recordType in stores).	new-cap, 1st option, properties
Use maximum line length of 100 characters.	Use maximum line length of 512 characters.	100 characters is on the small side, and 512 is the limit Open XDMoD's PHP code uses.	max-len, 1st option
Disallow usage of the continue keyword.	Allow usage of the continue keyword.	Appropriate usage of continue can significantly reduce indentation in lengthy and/or nested loops, making them easier to read.	no-continue
Disallow unary increments and decrements.	Allow unary increments and decrements.	The concerns that led to the creation of this rule are not applicable with the semicolon/spacing rules being used for this project.	no-plusplus
Disallow calling Object.prototype methods from Object instances.	Allow calling Object.prototype methods from Object instances.	The JavaScript feature described by the rule is not one we make use of, and even if it were, the error it attempts to prevent should be obvious when run.	no-prototype-builtins

Removals of preexisting custom rules in favor of Airbnb defaults are explained in the table below.

Old Custom Rule	Airbnb Rule	Explanation	ESLint Setting
Treat variables as if they were block scoped. (Warning)	Treat variables as if they were block scoped. (Error)	The only difference between the two rulesets was the severity of the error.	block-scoped-var
Disallow extra parentheses.	Allow extra parentheses.	Extra parentheses can be helpful for code readability.	no-extra-parens
Require dot notation for accessing object properties using static keys, except for keys matching regex ^[a-z]+(_[a-z]+)+$.	Require dot notation for accessing object properties using static keys.	Consistently using dot notation for static keys and bracket notation for dynamic keys improves code readability.	dot-notation

Tests Performed

Ran ESLint with new plugin and custom rules and verified that it worked as expected.

Checklist:

• My code follows the code style of this project as found in the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[smgallo]

Overall I think this looks good and is a step in the right direction. I did find some strange warnings that I couldn't figure out, though. For example, running on the /node_modules/.bin/eslint html/gui/js/modules/Usage.js produced this set of warnings:

  1252:21   warning  'parameters' used outside of binding context
  1252:21   warning  'parameters' used outside of binding context
  1255:21   warning  'parameters' used outside of binding context
  1255:21   warning  'parameters' used outside of binding context
  1257:21   warning  'parameters' used outside of binding context
  1257:21   warning  'parameters' used outside of binding context
  1263:17   warning  'parameters' used outside of binding context
  1263:17   warning  'parameters' used outside of binding context
  1264:17   warning  'parameters' used outside of binding context
  1264:17   warning  'parameters' used outside of binding context

On this block of code

                var parameters = [];

                if (n.attributes.node_type == 'statistic') {
                    parameters = getChartParameters(n);
                } else if (n.attributes.node_type == 'group_by') {
                    parameters = getMenuParameters(n);
                }

                //parameters['operation'] = 'get_param_descriptions';
                //parameterDescriptionStore.load({params: parameters});

                parameters.format = 'jsonstore';
                parameters.operation = 'get_data';

                viewGrid.store.proxy.setUrl('controllers/user_interface.php', true);
                viewGrid.store.load({
                    params: parameters
                });

This is in a function and I don't see parameters define previously in the function. The uses appear to be within scope unless I'm overlooking something.

[tyearke]

@smgallo Thanks!

I looked into that example and the problem is the rule's description doesn't match up with every case the rule can flag. As a secondary case, block-scoped-var checks for variables declared multiple times in the same scope but different blocks. The example they give is below.

// Incorrect
function doIfElse() {
    if (true) {
        var build = true;
    } else {
        var build = false;
    }
}

// Correct
function doIfElse() {
    var build;

    if (true) {
        build = true;
    } else {
        build = false;
    }
}

Example Source

Once I made a similar change to our code, the error went away.

I think the primary case the rule handles is worth keeping it for, but this secondary case should probably be broken out into its own rule. If C-style block scope is what this rule attempts to emulate, then the "incorrect" case is perfectly okay as long as the variables aren't used outside of those conditional blocks. If this case is one some developers want to disallow, then there should be a separate rule to tell the user exactly what they did wrong.

[plessbd]

Do we need corresponding pull request in any of the other repos to make sure we have the same styles across repositories?

[tyearke]

@plessbd We do. I was just waiting until there was consensus on the style before copying the rules over.

[plessbd]

In that case. The only one I would argue with is vars-on-top because of hoisting. However, i don't feel strongly enough to fight that hard for it.