From 32c892fe98ce3ff47c9092fbb84427c96f35fc20 Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Sat, 16 May 2020 15:56:30 -0700 Subject: [PATCH 1/7] Updated code standards --- CONTRIBUTING.md | 114 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 82 insertions(+), 32 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7e7e04102d..066dbcb835 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -122,43 +122,93 @@ changes. ### Code Standards -JavaScript sources in Open MCT must satisfy JSLint under its default -settings. This is verified by the command line build. +JavaScript sources in Open MCT must satisfy the ESLint rules defined in +this repository. This is verified by the command line build. #### Code Guidelines JavaScript sources in Open MCT should: -* Use four spaces for indentation. Tabs should not be used. -* Include JSDoc for any exposed API (e.g. public methods, constructors). -* Include non-JSDoc comments as-needed for explaining private variables, - methods, or algorithms when they are non-obvious. -* Define one public class per script, expressed as a constructor function - returned from an AMD-style module. -* Follow “Java-like” naming conventions. These includes: - * Classes should use camel case, first letter capitalized - (e.g. SomeClassName). - * Methods, variables, fields, and function names should use camel case, - first letter lower-case (e.g. someVariableName). - * Constants (variables or fields which are meant to be declared and - initialized statically, and never changed) should use only capital - letters, with underscores between words (e.g. SOME_CONSTANT). - * File names should be the name of the exported class, plus a .js extension - (e.g. SomeClassName.js). -* Avoid anonymous functions, except when functions are short (a few lines) - and/or their inclusion makes sense within the flow of the code - (e.g. as arguments to a forEach call). -* Avoid deep nesting (especially of functions), except where necessary - (e.g. due to closure scope). -* End with a single new-line character. -* Expose public methods by declaring them on the class's prototype. -* Within a given function's scope, do not mix declarations and imperative - code, and present these in the following order: - * First, variable declarations and initialization. - * Second, function declarations. - * Third, imperative statements. - * Finally, the returned value. - +1. Write clean code. Here’s a good summary - https://github.com/ryanmcdermott/clean-code-javascript. +1. Include JSDoc for any exposed API (e.g. public methods, constructors). +1. Include non-JSDoc comments as-needed for explaining private variables, + methods, or algorithms when they are non-obvious. Otherwise code + should be self-documenting. +1. Classes and Vue components should use camel case, first letter capitalized + (e.g. SomeClassName). +1. Methods, variables, fields, events, and function names should use camelCase, + first letter lower-case (e.g. someVariableName). +1. Source files that export functions should use camelCase, first letter lower-case (eg. testTools.js) +1. Constants (variables or fields which are meant to be declared and + initialized statically, and never changed) should use only capital + letters, with underscores between words (e.g. SOME_CONSTANT). They should always be declared as `CONST`s +1. File names should be the name of the exported class, plus a .js extension + (e.g. SomeClassName.js). +1. Avoid anonymous functions, except when functions are short (a few lines) + and/or their inclusion makes sense within the flow of the code + (e.g. as arguments to a forEach call). +1. Named functions are preferred over functions assigned to variables. +eg. +```JavaScript +function renameObject(object, newName) { + Object.name = newName; +} +``` +is preferable to +```JavaScript +const rename = (object, newName) => { + Object.name = newName; +} +``` +1. Avoid deep nesting (especially of functions), except where necessary + (e.g. due to closure scope). +1. End with a single new-line character. +1. Always use ES6 `Class`es and inheritence rather than the pre-ES6 prototypal + pattern. +1. Within a given function's scope, do not mix declarations and imperative + code, and present these in the following order: + * First, variable declarations and initialization. + * Secondly, imperative statements. + * Finally, the returned value. +1. Avoid the use of "magic" values. + eg. + ```JavaScript + Const UNAUTHORIZED = 401 + if (responseCode === UNAUTHORIZED) + ``` + is preferable to + ```JavaScript + if (responseCode === 401) + ``` +1. Don’t use the ternary operator. Yes it's terse, but there's probably a clearer way of writing it. +1. Test specs should reside alongside the source code they test, not in a separate directory. +1. Organize code by feature, not by type. +eg. +``` +- telemetryTable + - row + TableRow.js + TableRowCollection.js + TableRow.vue + - column + TableColumn.js + TableColumn.vue + plugin.js + pluginSpec.js +``` +is preferable to +``` +- telemetryTable + - components + TableRow.vue + TableColumn.vue + - collections + TableRowCollection.js + TableColumn.js + TableRow.js + plugin.js + pluginSpec.js +``` Deviations from Open MCT code style guidelines require two-party agreement, typically from the author of the change and its reviewer. From da2ecbbcad8ead3fdb8e4ec2871fc82d273e8f76 Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Sat, 16 May 2020 16:06:59 -0700 Subject: [PATCH 2/7] Fixed formatting issues, removed outdated example code. --- CONTRIBUTING.md | 119 ++++++++++++++++-------------------------------- 1 file changed, 38 insertions(+), 81 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 066dbcb835..9febd0dfaf 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -148,18 +148,18 @@ JavaScript sources in Open MCT should: and/or their inclusion makes sense within the flow of the code (e.g. as arguments to a forEach call). 1. Named functions are preferred over functions assigned to variables. -eg. -```JavaScript -function renameObject(object, newName) { - Object.name = newName; -} -``` -is preferable to -```JavaScript -const rename = (object, newName) => { - Object.name = newName; -} -``` + eg. + ```JavaScript + function renameObject(object, newName) { + Object.name = newName; + } + ``` + is preferable to + ```JavaScript + const rename = (object, newName) => { + Object.name = newName; + } + ``` 1. Avoid deep nesting (especially of functions), except where necessary (e.g. due to closure scope). 1. End with a single new-line character. @@ -183,78 +183,35 @@ const rename = (object, newName) => { 1. Don’t use the ternary operator. Yes it's terse, but there's probably a clearer way of writing it. 1. Test specs should reside alongside the source code they test, not in a separate directory. 1. Organize code by feature, not by type. -eg. -``` -- telemetryTable - - row - TableRow.js - TableRowCollection.js - TableRow.vue - - column - TableColumn.js - TableColumn.vue - plugin.js - pluginSpec.js -``` -is preferable to -``` -- telemetryTable - - components - TableRow.vue - TableColumn.vue - - collections - TableRowCollection.js - TableColumn.js - TableRow.js - plugin.js - pluginSpec.js -``` + eg. + ``` + - telemetryTable + - row + TableRow.js + TableRowCollection.js + TableRow.vue + - column + TableColumn.js + TableColumn.vue + plugin.js + pluginSpec.js + ``` + is preferable to + ``` + - telemetryTable + - components + TableRow.vue + TableColumn.vue + - collections + TableRowCollection.js + TableColumn.js + TableRow.js + plugin.js + pluginSpec.js + ``` Deviations from Open MCT code style guidelines require two-party agreement, typically from the author of the change and its reviewer. -#### Code Example - -```js -/*global define*/ - -/** - * Bundles should declare themselves as namespaces in whichever source - * file is most like the "main point of entry" to the bundle. - * @namespace some/bundle - */ -define( - ['./OtherClass'], - function (OtherClass) { - "use strict"; - - /** - * A summary of how to use this class goes here. - * - * @constructor - * @memberof some/bundle - */ - function ExampleClass() { - } - - // Methods which are not intended for external use should - // not have JSDoc (or should be marked @private) - ExampleClass.prototype.privateMethod = function () { - }; - - /** - * A summary of this method goes here. - * @param {number} n a parameter - * @returns {number} a return value - */ - ExampleClass.prototype.publicMethod = function (n) { - return n * 2; - } - - return ExampleClass; - } -); -``` - ### Test Standards Automated testing shall occur whenever changes are merged into the main From f1faf3965d32bd47db012ff108a94c4d555cf96a Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Sat, 16 May 2020 16:07:49 -0700 Subject: [PATCH 3/7] Changed reference to `constructors` to `classes` --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9febd0dfaf..8d091b00fd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -130,7 +130,7 @@ this repository. This is verified by the command line build. JavaScript sources in Open MCT should: 1. Write clean code. Here’s a good summary - https://github.com/ryanmcdermott/clean-code-javascript. -1. Include JSDoc for any exposed API (e.g. public methods, constructors). +1. Include JSDoc for any exposed API (e.g. public methods, classes). 1. Include non-JSDoc comments as-needed for explaining private variables, methods, or algorithms when they are non-obvious. Otherwise code should be self-documenting. From 41dc9c794d1a1bc98e907344b2930f8cd4eed27c Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Sat, 16 May 2020 16:08:43 -0700 Subject: [PATCH 4/7] Fixed capitalization on `CONST` --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8d091b00fd..fd25bbad3c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -141,7 +141,7 @@ JavaScript sources in Open MCT should: 1. Source files that export functions should use camelCase, first letter lower-case (eg. testTools.js) 1. Constants (variables or fields which are meant to be declared and initialized statically, and never changed) should use only capital - letters, with underscores between words (e.g. SOME_CONSTANT). They should always be declared as `CONST`s + letters, with underscores between words (e.g. SOME_CONSTANT). They should always be declared as `const`s 1. File names should be the name of the exported class, plus a .js extension (e.g. SomeClassName.js). 1. Avoid anonymous functions, except when functions are short (a few lines) From a40ff073536954be4a9ae2290c79d756051e2043 Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Sat, 16 May 2020 16:11:42 -0700 Subject: [PATCH 5/7] Updated guidance on anonymous functions --- CONTRIBUTING.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fd25bbad3c..dd21c97cb0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -144,9 +144,9 @@ JavaScript sources in Open MCT should: letters, with underscores between words (e.g. SOME_CONSTANT). They should always be declared as `const`s 1. File names should be the name of the exported class, plus a .js extension (e.g. SomeClassName.js). -1. Avoid anonymous functions, except when functions are short (a few lines) - and/or their inclusion makes sense within the flow of the code - (e.g. as arguments to a forEach call). +1. Avoid anonymous functions, except when functions are short (one or two lines) + and their inclusion makes sense within the flow of the code + (e.g. as arguments to a forEach call). Anonymous functions should always be arrow functions. 1. Named functions are preferred over functions assigned to variables. eg. ```JavaScript From 33faeafa98012e5138a1c56466917c1cf6e19601 Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Sat, 16 May 2020 16:19:36 -0700 Subject: [PATCH 6/7] New proposed ESLint rules that require no code changes --- .eslintrc.js | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/.eslintrc.js b/.eslintrc.js index 3cf7ec2e20..a65219828d 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -1,3 +1,4 @@ +const LEGACY_FILES = ["platform/**", "example/**"]; module.exports = { "env": { "browser": true, @@ -66,6 +67,56 @@ module.exports = { ], "dot-notation": "error", "indent": ["error", 4], + + // https://eslint.org/docs/rules/no-case-declarations + "no-case-declarations": "error", + // https://eslint.org/docs/rules/max-classes-per-file + "max-classes-per-file": ["error", 1], + // https://eslint.org/docs/rules/no-eq-null + "no-eq-null": "error", + // https://eslint.org/docs/rules/no-eval + "no-eval": "error", + // https://eslint.org/docs/rules/no-floating-decimal + "no-floating-decimal": "error", + // https://eslint.org/docs/rules/no-implicit-globals + "no-implicit-globals": "error", + // https://eslint.org/docs/rules/no-implied-eval + "no-implied-eval": "error", + // https://eslint.org/docs/rules/no-lone-blocks + "no-lone-blocks": "error", + // https://eslint.org/docs/rules/no-loop-func + "no-loop-func": "error", + // https://eslint.org/docs/rules/no-new-func + "no-new-func": "error", + // https://eslint.org/docs/rules/no-new-wrappers + "no-new-wrappers": "error", + // https://eslint.org/docs/rules/no-octal-escape + "no-octal-escape": "error", + // https://eslint.org/docs/rules/no-proto + "no-proto": "error", + // https://eslint.org/docs/rules/no-return-await + "no-return-await": "error", + // https://eslint.org/docs/rules/no-script-url + "no-script-url": "error", + // https://eslint.org/docs/rules/no-self-compare + "no-self-compare": "error", + // https://eslint.org/docs/rules/no-sequences + "no-sequences": "error", + // https://eslint.org/docs/rules/no-unmodified-loop-condition + "no-unmodified-loop-condition": "error", + // https://eslint.org/docs/rules/no-useless-call + "no-useless-call": "error", + // https://eslint.org/docs/rules/wrap-iife + "wrap-iife": "error", + // https://eslint.org/docs/rules/no-nested-ternary + "no-nested-ternary": "error", + // https://eslint.org/docs/rules/switch-colon-spacing + "switch-colon-spacing": "error", + // https://eslint.org/docs/rules/no-useless-computed-key + "no-useless-computed-key": "error", + // https://eslint.org/docs/rules/rest-spread-spacing + "rest-spread-spacing": ["error"], + "vue/html-indent": [ "error", 4, @@ -112,6 +163,13 @@ module.exports = { } ] } + }, { + "files": LEGACY_FILES, + "rules": { + // https://eslint.org/docs/rules/no-nested-ternary + "no-nested-ternary": "off", + "no-var": "off" + } } ] }; From d6bb1b2a12768fce79e510867774bfbf5f5b0744 Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Tue, 2 Jun 2020 13:05:15 -0700 Subject: [PATCH 7/7] Added note about requiring a single return statement. --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index dd21c97cb0..8605ba3002 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -169,7 +169,7 @@ JavaScript sources in Open MCT should: code, and present these in the following order: * First, variable declarations and initialization. * Secondly, imperative statements. - * Finally, the returned value. + * Finally, the returned value. Functions should only have a single return statement. 1. Avoid the use of "magic" values. eg. ```JavaScript