Use shared lint config: eslint-config-scratch #298

rschamp · 2016-10-24T03:29:29Z

Towards more consistent style across our JS projects, I've added a centralized ESLint configuration at https://github.com/LLK/eslint-config-scratch. This PR installs the config and updates the syntax to match its expectations.

Since it's a centralized config to be used in all our projects, the rules should be a collective decision so I assigned a bunch of people who might care. If any of these changes annoy you, let's talk about it. I just created it based on what I thought made sense, and then removed rules as I tried to lint this repo.

A few of the rules that required most of the changes:

'func-style': ['error', 'expression']: Functions should be written in the form var functionName = function () {}. This ensures that functions are defined before they're used, which makes code easier to understand as it's read. It also forces the code to be written as it will be executed. The declaration style function functionName () {} relies on functions to be automatically hoisted to the top of the file.
'space-before-function-paren': ['error', 'always']: This is the style we've been using elsewhere. Thankfully you can automatically apply this rule to existing code by running eslint with the --fix argument.
'camelcase': ['error']: All variables should be written in "camelCase" style, without underscores. This improves consistency of naming. There is a convention in this codebase to name options with a mix of underscore and camelcase, like opt_optionName. These have been renamed optOptionName. Unfortunately there's no way to configure a pattern for allowed variable names to include the opt_ prefix.
'eqeqeq': ['error']: Always using === or !== leads to fewer "wat?" errors
'no-undefined': ['error']: Don't use undefined. This rule is mostly to ensure consistency in syntax across the projects in the way we use undefined, but also avoids similar errors to those avoided by the === rule.

Resolved whitespace issues, e.g., space-before-function-paren and key-spacing

rschamp

I tried to explain some of the other fixes required for the new rules. The last one I left out was using minilog rather than console. This is mostly for consistency, but also gives us the ability to log somewhere other than the console if/when we want to do that, and prefixes the messages with vm, which will be useful when we have messages coming from many different projects at once.

src/engine/execute.js

            if (runtime.ioDevices[device] && runtime.ioDevices[device][func]) {
                var devObject = runtime.ioDevices[device];
-                return devObject[func].call(devObject, args);
+                return devObject[func](args);


src/blocks/scratch3_motion.js

+    var distLeft = Math.max(0, (stageWidth / 2) + bounds.left);
+    var distTop = Math.max(0, (stageHeight / 2) - bounds.top);
+    var distRight = Math.max(0, (stageWidth / 2) - bounds.right);
+    var distBottom = Math.max(0, (stageHeight / 2) + bounds.bottom);


src/blocks/scratch3_operators.js

    // If both arguments are ints, truncate the result to an int.
    if (Cast.isInt(args.FROM) && Cast.isInt(args.TO)) {
-        return low + parseInt(Math.random() * ((high + 1) - low));
+        return low + parseInt(Math.random() * ((high + 1) - low), 10);


src/engine/adapter.js

+    if (typeof e !== 'object') return;
+    if (typeof e.xml !== 'object') return;
+
+    return domToBlocks(html.parseDOM(e.xml.outerHTML));


src/engine/execute.js

-                if (!resolvedValue) {
-                    sequencer.retireThread(thread);
-                }
+                sequencer.retireThread(thread);


src/engine/execute.js

-        if (typeof currentStackFrame.reported[inputName] === 'undefined'
-            && inputBlockId) {
+        if (typeof currentStackFrame.reported[inputName] === 'undefined' &&
+            inputBlockId) {


src/import/sb2import.js

    var inputCount = 0;
    // Split by %n, %b, %s.
-    var parts = procCode.split(/(?=[^\\]\%[nbs])/);
+    var parts = procCode.split(/(?=[^\\]%[nbs])/);


thisandagain

Thanks for pulling this together @rschamp. Mostly looks good though I left a few notes inline.

.eslintrc.js

+module.exports = {
+    extends: ['scratch', 'scratch/node'],
+    env: {
+        node: true


src/blocks/scratch3_control.js

 var Timer = require('../util/timer');

-function Scratch3ControlBlocks(runtime) {
+var Scratch3ControlBlocks = function (runtime) {


src/blocks/scratch3_motion.js

+    var distLeft = Math.max(0, (stageWidth / 2) + bounds.left);
+    var distTop = Math.max(0, (stageHeight / 2) - bounds.top);
+    var distRight = Math.max(0, (stageWidth / 2) - bounds.right);
+    var distBottom = Math.max(0, (stageHeight / 2) + bounds.bottom);


src/blocks/scratch3_control.js

    var times = Math.floor(Cast.toNumber(args.TIMES));
    // Initialize loop
-    if (util.stackFrame.loopCounter === undefined) {
+    if (typeof util.stackFrame.loopCounter === 'undefined') {


src/blocks/scratch3_control.js

+Scratch3ControlBlocks.prototype.stop = function (args, util) {
    var option = args.STOP_OPTION;
-    if (option == 'all') {
+    if (option === 'all') {


src/engine/blocks.js

    var tagName = (block.shadow) ? 'shadow' : 'block';
    var xy = (block.topLevel) ?
-        ' x="' + block.x +'"' + ' y="' + block.y +'"' :
+        ' x="' + block.x +'" y="' + block.y +'"' :


src/engine/execute.js

-                if (!resolvedValue) {
-                    sequencer.retireThread(thread);
-                }
+                sequencer.retireThread(thread);


src/engine/list.js

  * @constructor
  */
-function List (name, contents) {
+module.exports = function List (name, contents) {


test/unit/util_color.js

-    t.deepEqual(color.decimalToRgb(16777215), {r:255,g:255,b:255});
-    t.deepEqual(color.decimalToRgb(-16777215), {r:0,g:0,b:1});
-    t.deepEqual(color.decimalToRgb(99999999), {r:245,g:224,b:255});
+    t.deepEqual(color.decimalToRgb(0), {r: 0,g: 0,b: 0});


test/.eslintrc.js

@@ -0,0 +1,5 @@
+module.exports = {


tmickel

Looks sweet to me, thanks for doing all the work to investigate and apply these!!!

src/blocks/scratch3_looks.js

 * @param {!Target} target Target to set costume/backdrop to.
 * @param {Any} requestedCostume Costume requested, e.g., 0, 'name', etc.
- * @param {boolean=} opt_zeroIndex Set to zero-index the requestedCostume.
+ * @param {boolean=} optZeroIndex Set to zero-index the requestedCostume.


src/blocks/scratch3_motion.js

+    var distLeft = Math.max(0, (stageWidth / 2) + bounds.left);
+    var distTop = Math.max(0, (stageHeight / 2) - bounds.top);
+    var distRight = Math.max(0, (stageWidth / 2) - bounds.right);
+    var distBottom = Math.max(0, (stageHeight / 2) + bounds.bottom);


src/engine/execute.js


    if (!opcode) {
-        console.warn('Could not get opcode for block: ' + currentBlockId);
+        log.warn('Could not get opcode for block: ' + currentBlockId);


src/engine/list.js

  * @constructor
  */
-function List (name, contents) {
+module.exports = function List (name, contents) {


src/import/sb2specmap.js

-                'inputName':'BEATS'
+                'type': 'input',
+                'inputOp': 'math_number',
+                'inputName': 'BEATS'


http://eslint.org/docs/rules/space-infix-ops

We are on a fast release cycle here.

src/blocks/scratch3_motion.js

+        motion_pointtowards: this.pointTowards,
+        motion_glidesecstoxy: this.glide,
+        motion_ifonedgebounce: this.ifOnEdgeBounce,
+        motion_setrotationstyle: this.setRotationStyle,


rschamp · 2016-10-24T15:42:00Z

Updated spaces around infix operators and enforced consistent quotes around object properties: if any of the properties need quotes, then they all need quotes. Otherwise none of them should use quotes. Also fixed the node env config @thisandagain pointed out.

thisandagain · 2016-10-24T15:48:21Z

I think the only question I have left is the func-style rule / pattern consistency. Otherwise this looks 👍 to me. Any thoughts @mewtaylor @cwillisf ?

rschamp · 2016-10-24T15:50:26Z

@thisandagain Updated to the var Foo = function () {}; ... module.exports = Foo style, this is what we mostly use elsewhere.

mewtaylor

Thanks for this! With the suggestions made by others, this looks great!

cwillisf

Overall 👍 but I'm unsure about the var vs. function change and I think there's a typo in the latest version of the devObject[func] stuff.

Thanks for doing all this work! I look forward to improved consistency across our Scratch 3.0-related repositories :D

.eslintrc.js

@@ -0,0 +1,3 @@
+module.exports = {
+    extends: ['scratch', 'scratch/node']


src/blocks/scratch3_control.js

 var Timer = require('../util/timer');

-function Scratch3ControlBlocks(runtime) {
+var Scratch3ControlBlocks = function (runtime) {


src/engine/sequencer.js

        if (doWarp) {
            thread.peekStackFrame().warpMode = true;
-        } else {
+        } else if (isRecursive) {


src/engine/execute.js

+                // expressions... or something. Not exactly sure why it
+                // complains here.
+                // eslint-disable-next-line no-useless-call
+                return devObject[func].call(devObject[func], args);


thisandagain

Thanks for wrangling all this @rschamp

Ray Schamp added 5 commits October 23, 2016 12:03

Install and use eslint-config-scratch

36e4683

Pass with eslint --fix

68ed110

Resolved whitespace issues, e.g., space-before-function-paren and key-spacing

All linting other than console statements

f6c0064

Replace console with minilog

73edb65

Keep up with non-breaking lint changes

1bd1d8e

rschamp added the pr - needs review label Oct 24, 2016

rschamp assigned tmickel, thisandagain, mewtaylor and cwillisf Oct 24, 2016

rschamp commented Oct 24, 2016

View reviewed changes

thisandagain reviewed Oct 24, 2016

View reviewed changes

test/.eslintrc.js

@@ -0,0 +1,5 @@

module.exports = {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

tmickel approved these changes Oct 24, 2016

View reviewed changes

tmickel removed their assignment Oct 24, 2016

Ray Schamp added 4 commits October 24, 2016 10:28

Revert no-lonely-if

14a2dbc

Fix comma-spacing

0e909cb

Fix quote-props

7712b29

Fix space-infix-ops

63cfa42

http://eslint.org/docs/rules/space-infix-ops

rschamp mentioned this pull request Oct 24, 2016

Rule fixes post scratch-vm review scratchfoundation/eslint-config-scratch#1

Merged

Update to eslint-config-scratch 2.0

9f0c4eb

We are on a fast release cycle here.

tmickel reviewed Oct 24, 2016

View reviewed changes

Use consistent function declaration style

04d7f4f

mewtaylor approved these changes Oct 24, 2016

View reviewed changes

mewtaylor removed their assignment Oct 24, 2016

Ray Schamp added 2 commits October 24, 2016 13:01

Revert no-lonely-if

22bf947

Disable no-useless-call in case of behavior change

d2a0e6f

cwillisf requested changes Oct 24, 2016

View reviewed changes

Fix typo

659014b

thisandagain approved these changes Oct 24, 2016

View reviewed changes

thisandagain removed their assignment Oct 24, 2016

cwillisf approved these changes Oct 24, 2016

View reviewed changes

cwillisf added pr - good to merge and removed pr - needs review labels Oct 24, 2016

cwillisf assigned rschamp and unassigned cwillisf Oct 24, 2016

rschamp merged commit 595be5c into scratchfoundation:develop Oct 24, 2016

rschamp deleted the scratch-lint branch October 24, 2016 19:43

		@@ -0,0 +1,3 @@
		module.exports = {
		extends: ['scratch', 'scratch/node']

Use shared lint config: eslint-config-scratch #298

Use shared lint config: eslint-config-scratch #298

Uh oh!

Conversation

rschamp commented Oct 24, 2016

Uh oh!

rschamp left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

thisandagain left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

tmickel left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.