Rule Enhancement Proposal: object-property-newline with never and some options

[mysticatea]

From disallowObjectKeysOnNewLine.

I had forgotten to make this proposal in my previous vacation.

JSCS has disallowObjectKeysOnNewLine rule, but object-property-newline rule doesn't support it, so I think we need to add "always"/"never" and some other options into object-property-newline rule. It would be similar options to #6072, #6073, and #6075. These options are "conditions to need line breaks". I like consistency, so I hope this proposal's option will be so also.

There are too many options in this proposal.
In my head, those are for autofix.

---

(fixable) The --fix option on the command line automatically fixes problems reported by this rule.

Options

{
    "object-property-newline": [
        "error",
        {"allowMultiplePropertiesPerLine": false}
    ],
    // OR
    "object-property-newline": [
        "error",
        "always" or "never" or {"multiline": false, "minProperties": 0, "minNonShorthandProperties": 0},
        {"allowMultiplePropertiesPerLine": false}
    ]
}

1st option: when does it require line breaks between properties?

• "always" (default) - requires line breaks always.
• "never" - disallows line breaks always.
• An object - requires line breaks if any of the following conditions are satisfied. Otherwise, disallows line breaks.
  • multiline (default is false) - requires line breaks if there are line breaks inside of properties. If this is false, this condition is disabled.
  • minProperties (default is 0) - requires line breaks if the number of properties is more than the given integer. If this is 0, this condition is disabled.
  • minLongformProperties (default is 0) - requires line breaks if the number of non-shorthand properties is more than the given integer. If this is 0, this condition is disabled. (object-property-newline with enhanced object literals #6211)

2nd option:

• allowMultiplePropertiesPerLine (default is false) - allows all keys and values to be on the same line.

always

Examples of incorrect code for this rule with the "always" option:

/*eslint object-property-newline: ["error", "always"]*/
/*eslint-env es6*/

let c = {foo: 1, bar: 2};
let d = {foo: 1, bar: 2, baz: 3};
let e = {foo, bar, baz};
let f = {
    foo, bar() {
        dosomething();
    }
};

Examples of correct code for this rule with the "always" option:

/*eslint object-property-newline: ["error", "always"]*/
/*eslint-env es6*/

let a = {};
let b = {foo: 1};
let c = {foo: 1,
    bar: 2};
let d = {foo: 1,
    bar: 2,
    baz: 3};
let e = {foo,
    bar,
    baz};
let f = {
    foo,
    bar() {
        dosomething();
    }
};

never

Examples of incorrect code for this rule with the "never" option:

/*eslint object-property-newline: ["error", "never"]*/
/*eslint-env es6*/

let c = {foo: 1,
    bar: 2};
let d = {foo: 1,
    bar: 2,
    baz: 3};
let e = {foo,
    bar,
    baz};
let f = {
    foo,
    bar() {
        dosomething();
    }
};

Examples of correct code for this rule with the "never" option:

/*eslint object-property-newline: ["error", "never"]*/
/*eslint-env es6*/

let a = {};
let b = {foo: 1};
let c = {foo: 1, bar: 2};
let d = {foo: 1, bar: 2, baz: 3};
let e = {foo, bar, baz};
let f = {
    foo, bar() {
        dosomething();
    }
};

multiline

Examples of incorrect code for this rule with the {"multiline": true} option:

/*eslint object-property-newline: ["error", {"multiline": true}]*/
/*eslint-env es6*/

let c = {foo: 1,
    bar: 2};
let d = {foo: 1,
    bar: 2,
    baz: 3};
let e = {foo,
    bar,
    baz};
let f = {
    foo, bar() {
        dosomething();
    }
};

Examples of correct code for this rule with the {"multiline": true} option:

/*eslint object-property-newline: ["error", {"multiline": true}]*/
/*eslint-env es6*/

let a = {};
let b = {foo: 1};
let c = {foo: 1, bar: 2};
let d = {foo: 1, bar: 2, baz: 3};
let e = {foo, bar, baz};
let f = {
    foo,
    bar() {
        dosomething();
    }
};

minProperties

Examples of incorrect code for this rule with the {"minProperties": 3} option:

/*eslint object-property-newline: ["error", {"minProperties": 3}]*/
/*eslint-env es6*/

let c = {foo: 1,
    bar: 2};
let d = {foo: 1, bar: 2, baz: 3};
let e = {foo, bar, baz};
let f = {
    foo,
    bar() {
        dosomething();
    }
};

Examples of correct code for this rule with the {"minProperties": 3} option:

/*eslint object-property-newline: ["error", {"minProperties": 3}]*/
/*eslint-env es6*/

let a = {};
let b = {foo: 1};
let c = {foo: 1, bar: 2};
let d = {foo: 1,
    bar: 2,
    baz: 3};
let e = {foo,
    bar,
    baz};
let f = {
    foo, bar() {
        dosomething();
    }
};

minLongformProperties

Examples of incorrect code for this rule with the {"minLongformProperties": 3} option:

/*eslint object-property-newline: ["error", {"minLongformProperties": 3}]*/
/*eslint-env es6*/

let c = {foo: 1,
    bar: 2};
let d = {foo: 1, bar: 2, baz: 3};
let e = {foo,
    bar,
    baz};
let f = {
    foo,
    bar() {
        dosomething();
    }
};

Examples of correct code for this rule with the {"minLongformProperties": 3} option:

/*eslint object-property-newline: ["error", {"minLongformProperties": 3}]*/
/*eslint-env es6*/

let a = {};
let b = {foo: 1};
let c = {foo: 1, bar: 2};
let d = {foo: 1,
    bar: 2,
    baz: 3};
let e = {foo, bar, baz};
let f = {
    foo, bar() {
        dosomething();
    }
};

a combination of multiline and minLongformProperties

Examples of incorrect code for this rule with the {"multiline": true, "minLongformProperties": 3} option:

/*eslint object-property-newline: ["error", {"multiline": true, "minLongformProperties": 3}]*/
/*eslint-env es6*/

let c = {foo: 1,
    bar: 2};
let d = {foo: 1, bar: 2, baz: 3};
let e = {foo,
    bar,
    baz};
let f = {
    foo, bar() {
        dosomething();
    }
};

Examples of correct code for this rule with the {"multiline": true, "minLongformProperties": 3} option:

/*eslint object-property-newline: ["error", {"multiline": true, "minLongformProperties": 3}]*/
/*eslint-env es6*/

let a = {};
let b = {foo: 1};
let c = {foo: 1, bar: 2};
let d = {foo: 1,
    bar: 2,
    baz: 3};
let e = {foo, bar, baz};
let f = {
    foo,
    bar() {
        dosomething();
    }
};

Related Rules

	brace style	space style	line break style
block	brace-style	block-spacing	max-statements-per-line
object	#6072 object-curly-newline	object-curly-spacing	object-property-newline
array	#6073 array-bracket-newline	array-bracket-spacing	#6075 array-element-newline

• comma-spacing

[mysticatea]

Ugh, I had gotten small trouble of labels and milestone by restoration of closed tabs.

[jamestalmage]

I like where you are headed with this.

What does the allowMultiplePropertiesPerLine do? How is that not covered by setting limits for shorthand and longform properties?

I think some of the option names could be improved:

Is minXX be better as maxXXX? You are specifying the maximum number allowed per line.

multiline is a bit ambiguous too:

• Are you referring to a line split between key and value:

  key:
    value
  

• Or a value which spans multiple lines

  key: function value() {
  }
  

I believe you intended the latter, so maybe it should be multiLineValue?

[mysticatea]

Thank you!

What does the allowMultiplePropertiesPerLine do? How is that not covered by setting limits for shorthand and longform properties?

Probably.

Is minXX be better as maxXXX? You are specifying the maximum number allowed per line.

I thought those options are "conditions to need line breaks".

• Always requires line breaks.
• Never requires line breaks.
• If it's multiline, requires line breaks.
• If it has more properties than minProperties, requires line breaks...

I'm not native. Is this strange? (but I have released object-curly-newline with those name...)

multiline is a bit ambiguous too:

Indeed.
My intention was the former, "if any Property node is multiline, requires line breaks.".
The following pattern are multiline.

let obj = {
    [
        key
    ]: value
}
let obj = {
    key:
        value
}
let obj = {
    key() {
    }
}

[platinumazure]

@eslint/eslint-team Can we get more eyes on this proposal?

I'm a little leery of the number of options (only in terms of, is this worth doing as multiple issues/PRs?), but that's not a huge issue. But we should really make sure we get the option names right.

I'm going to offer one slightly different schema: In order to emphasize that most of the options are conditions for requiring the property newline, maybe they could be grouped in an object with a key indicating that? (I've also changed some of the other keys, but I'm not strongly attached to these names.)

{
    "object-property-newline": ["error", {
        "requiredWhen": {
            "propertyCount": 3,           // Equivalent to minProperties
            "longformPropertyCount": 3,   // Equivalent to minLongformProperties
            "hasMultilineProperty": true  // Equivalent to multiline
        }
    }
}

[vitorbal]

@platinumazure cool, i like your schema proposal, it reads very naturally. I was able to read it after a long time not following this issue and pretty much guess what the options would do.

[flying-sheep]

I would like to see an additional extension: longform. (or requiredWhen: { isLongform: true })

Examples of correct code for this rule with the {"longform": true } option:

let obj = {
	foo, bar, baz,
	zug: 'flob',
	nad: 'ban',
}

Examples of incorrect code for this rule with the {"longform": true } option:

let obj = {  //shorthand inconsistent
	foo, bar,
	baz,
	zug: 'flob',
	nad: 'ban',
}

obj = {  //longform not on individual lines
	foo, bar, baz, zug: 'flob', nad: 'ban',
}

---

i think there should be a way to say that those three are OK, but i can’t think the best option:

let obj = {
	foo,
	bar,
	baz,
	zug: 'flob',
	nad: 'ban',
}

obj = {
	foo, bar, baz,
	zug: 'flob',
	nad: 'ban',
}

obj = { foo, bar, baz, zug: 'flob', nad: 'ban' }

[kaicataldo]

@eslint/eslint-team Can we get some opinions to move this forward so we can start implementing?

[tandrewnichols]

What about an option to ignore this on default params? I have object-property-newline set to error, but I actually don't want that for default params because I think breaking a parameter list across lines seems a bit weird. Ideally, I would like to be able to do:

function(config = { foo: 'bar', baz: 'quux' })

but still get an error on

let obj = { foo: 'bar', baz: 'quux' }

[jrpool]

I am considering contributing on this issue. I should be able to make a decision within 2 days.

[jrpool]

After an initial review, I propose to liquidate this issue with the following steps:

1. Revise the user documentation on the existing rule object-property-newline to cover cases not documented and more fully describe the behavior of the --fix option.

2. Revise the tests of the existing rule object-property-newline to cover cases handled but not tested.

3. Evaluate whether the proposed rule should be a modification of the existing rule object-property-newline or a distinct new rule.

4. Implement the proposal insofar as necessary to produce equivalent functionality to JSCS rule disallowObjectKeysOnNewLine.

5. Implement the remainder of the proposal.

If there is any disagreement with this, please let me know. Thanks!

[jrpool]

Request for guidance:

Further review has made it seem appropriate to add one step (step 3 below) to the plan proposed above, resulting in the following:

1. Revise the user documentation on the existing rule object-property-newline to cover cases not documented and more fully describe the behavior of the --fix option. (done with PR Docs: Amend rule document to correct and complete it (refs #6251). #9498)

2. Revise the tests of the existing rule object-property-newline to cover cases handled but not tested. (Done with PR Chore: Add tests to cover array and object values and leading commas. #9502)

3. Add object options (and tests and documentation) to rule object-property-newline to complete compatibility with JSCS rule requireObjectKeysOnNewLine. (Done with PR New: Add object options to object-property-newline for JSCS. #9581)

4. Evaluate whether the proposed rule should be a modification of the existing rule object-property-newline or a distinct new rule. (Result: It should be a modification of the object-property-newline rule.)

5. Implement the proposal insofar as necessary to produce equivalent functionality to JSCS rule disallowObjectKeysOnNewLine. (Currently working on this)

6. Implement the remainder of the proposal.

Is the newly inserted step 3 appropriate?

[jrpool]

A related discussion is at https://www.bountysource.com/issues/32238926-port-jscs-requireobjectkeysonnewline-rule.

[jrpool]

Are misnamed object options ever deprecated and given new names? I’m thinking of this object-property-newline rule’s option named allowMultiplePropertiesPerLine, which actually means allowAllPropertiesOnSameLine.

[kaicataldo]

@jrpool Sorry, will try to respond to your other comments soon. As to your last question, we do deprecate configuration names in favor of other ones as long as they are backwards compatible.

[jrpool]

Thanks, @kaicataldo. That helps. Would that mean introducing a synonymous preferred name with identical behavior?

[jrpool]

I am inclined to pause the work on step 5 above until PR #9581 is acted on, because it is somewhat dysfunctional to implement step 5 without step 3. PR #9570 also affects this rule, and work on step 5 will be easier if that PR has also been acted on.

[nijikokun]

What about spaces between properties as well? Will the padding features be affected by this or has that not been taken into consideration?

For example, rules to handle such a case:

let object = {
  type: Boolean,
  property: {
    key: 'value'
  },
  another: {
    key: 'value'
  },
  key: function () {
    // ...
  }
}

VS:

let object = {
  type: Boolean,

  property: {
    key: 'value'
  },

  another: {
    key: 'value'
  },

  key: function () {
    // ...
  }
}

[jrpool]

In reply to @nijikokun, the current tests do not include cases with blank lines inside object literals, but I have added tests with such blank lines and verified that all the tests still pass. So the two examples that you provide seem to be indistinguishable, i.e. equivalent, to ESLint.

[nijikokun]

@jrpool that is good to know, it would be cool if eslint could distinguish them so we can configure preferences for one or the other.

[nzakas]

Unfortunately, it looks like there wasn't enough interest from the team or community to implement this change. While we wish we'd be able to accommodate everyone's requests, we do need to prioritize. We've found that issues failing to be implemented after 90 days tend to never be implemented, and as such, we close those issues. This doesn't mean the idea isn't interesting or useful, just that it's not something the team can commit to.

[platinumazure]

@nzakas This is a JSCS compatibility issue. Do we want to reopen and try to get this done?

[nzakas]

As this hasn't already been addressed, it seems like it's not very important for JSCS compatibility, so I'd say let's keep it closed unless we hear otherwise.

…

On Fri, Sep 21, 2018 at 8:42 AM Kevin Partington ***@***.***> wrote: @nzakas <https://github.com/nzakas> This is a JSCS compatibility issue. Do we want to reopen and try to get this done? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6251 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACWkgxz_-iSlhh4UR5tOrwJNvtwiRPxks5udQjygaJpZM4Imb1a> .

--

______________________________ Nicholas C. Zakas @SlickNet Author, Principles of Object-Oriented JavaScript <http://amzn.to/29Pmfrm> Author, Understanding ECMAScript 6 <http://amzn.to/29K1mIy>

[platinumazure]

Works for me. I was concerned about tracking this but the issue still shows up in the JSCS Compatibility project, so if someone decides to jump back to this later, they can. Happy to leave it closed.

[nijikokun]

Rip