key-spacing: multiLine.align.mode is overridden, multiLine.mode with align causes an error

[whizark]

What version of ESLint are you using?
ESLint 3.1.0

What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:

case1.js

module.exports = {
    "rules"   : {
        "key-spacing"                  : [
            "error",
            {
                "multiLine" : {
                    "beforeColon": false,
                    "afterColon" : true,
                    "mode"       : "strict",
                    "align"      : {
                        "beforeColon": false,
                        "afterColon" : true,
                        "on"         : "colon",
                        "mode"       : "minimum"
                    }
                }
            }
        ]
    }
};

case2.js

module.exports = {
    "rules": {
        "key-spacing": [
            "error",
            {
                "multiLine": {
                    "beforeColon": false,
                    "afterColon" : true,
                    "mode"       : "strict"
                },
                "align"    : {
                    "beforeColon": false,
                    "afterColon" : true,
                    "on"         : "colon",
                    "mode"       : "minimum"
                }
            }
        ]
    }
};

What did you do? Please include the actual source code causing the issue.

test.js

var a = {
    foo   : 1,
    align1: {
        foo:    2
    },
    align2: {
        foo:    3
    },
    bar: 4
};

What did you expect to happen?

Case 1

  5:5   warning  Unexpected a line break before this close brace  object-curly-newline
  6:13  warning  Unexpected a line break after this open brace    object-curly-newline

Case2

  1:1   error    Unexpected var, use let or const instead         no-var
  1:5   error    'a' is defined but never used                    no-unused-vars
  3:13  warning  Unexpected a line break after this open brace    object-curly-newline
  5:5   warning  Unexpected a line break before this close brace  object-curly-newline
  6:13  warning  Unexpected a line break after this open brace    object-curly-newline
  8:5   warning  Unexpected a line break before this close brace  object-curly-newline

What actually happened? Please include the actual, raw output from ESLint.

Case 1

  4:17  error    Extra space before value for key 'foo'           key-spacing
  5:5   warning  Unexpected a line break before this close brace  object-curly-newline
  6:13  warning  Unexpected a line break after this open brace    object-curly-newline
  7:17  error    Extra space before value for key 'foo'           key-spacing

Because of

eslint/lib/rules/key-spacing.js

Line 139 in 67c3cc2

mode: toOptions.multiLine.mode,

Case2

/path/to/config/case2.js:
        Configuration for rule "key-spacing" is invalid:
        Value "[object Object]" no schemas match.

Error: /path/to/config/case2.js:
        Configuration for rule "key-spacing" is invalid:
        Value "[object Object]" no schemas match.

Because of

eslint/lib/rules/key-spacing.js

Lines 271 to 321 in 67c3cc2

	{
	type: "object",
	properties: {
	singleLine: {
	type: "object",
	properties: {
	mode: {
	enum: ["strict", "minimum"]
	},
	beforeColon: {
	type: "boolean"
	},
	afterColon: {
	type: "boolean"
	}
	},
	additionalProperties: false
	},
	multiLine: {
	type: "object",
	properties: {
	beforeColon: {
	type: "boolean"
	},
	afterColon: {
	type: "boolean"
	}
	},
	additionalProperties: false
	},
	align: {
	type: "object",
	properties: {
	mode: {
	enum: ["strict", "minimum"]
	},
	on: {
	enum: ["colon", "value"]
	},
	beforeColon: {
	type: "boolean"
	},
	afterColon: {
	type: "boolean"
	}
	},
	additionalProperties: false
	}
	},
	additionalProperties: false
	}

(multiLine.mode isn't allowed with align option)

Are they designed as they are?

[btmills]

The object align option applies to properties that are in a group and can be aligned. Try the case1.js config on this:

var a = {
    foo   :    1,
    align1:    {
        foo: 2
    },
    align2: {
        foo: 3
    },
    bar: 4
};

Since foo and align1 are the only properties on adjacent lines in the same object literal, they're the only ones where align applies. All of the other properties are individual and will follow the rules for a multiLine object literal. Assuming that modified example passes, I think we're good on that part.

As for the schema error, that is a bug. key-spacing has a pretty complex schema, so mode was probably omitted by mistake.

[whizark]

@btmills Thank you for you explanation. I had a slight misunderstanding about align... 😳
I tried to lint the first code in align and multiLine with case1.js config and the code actually passed, so I also tried to lint another code based on the code above.

var myObj = {
  key1: 1, // uses multiLine

  key2:    2, // uses align (when defined)
  key3:    3, // uses align (when defined)

  key4: 4 // uses multiLine
}

The result is:

  4:12  error  Extra space before value for key 'key2'  key-spacing
  5:12  error  Extra space before value for key 'key3'  key-spacing

I'm a little confused because multiLine.align.mode in case1.js seems to be overridden with multiLine.mode. Is this also a bug? Or we cannot use both multiLine.mode and multiLine.align.mode in the same time?

[btmills]

@whizark is right - align isn't respecting its own mode at all, and is instead using multiLine's mode value.

Two steps to fixing this:

1. Change the schema to allow mode.align. Right now just multiLine.align.mode is permitted.
2. Change the code so that mode uses align's setting, if present, when properties are part of a group.

[annie]

i'll work on this!

[btmills]

Great, @azhang496 - let us know if you need any help!

[annie]

sorry for the delay - @btmills is it necessary to change the schema? seems like this can be fixed just by changing this

eslint/lib/rules/key-spacing.js

Line 139 in 67c3cc2

mode: toOptions.multiLine.mode,

to mode: toOptions.multiline.align.mode

[btmills]

@azhang496 I'm on my phone, so I can't check to be sure, but if the scheme is left unchanged, won't the case 2 config (setting a mode under align) still throw a validation error?

It looks like your suggestion would fix case 1 👍

If you want to tackle one at a time, a first PR with (refs #6691) would be fine.

[annie]

@btmills i can do both! i didn't realize the goal was to fix both cases, since just fixing case 1 would give the option to have different modes for multiline and align

[annie]

@btmills made a PR if you'd like to check it out