New: Add object options to object-property-newline for JSCS.

[jrpool]

New options make the rule compatible with JSCS rule requireObjectKeysOnNewLine.
Need for noCommaFirst option, instead of comma-style, explained in the rule document. Partly fixes issue #6251.

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What rule do you want to change?
object-property-newline

Does this change cause the rule to produce more or fewer warnings?
No.

How will the change be implemented? (New option, new default behavior, etc.)?
2 new options.

Please provide some example code that this change will affect:

const newObject = {
    a: 1, [
        process.argv[4]
    ]: '01'
};

What does the rule currently do for this code?
Treats it as invalid.

What will the rule do after it's changed?
Treat it as valid if the treatComputedPropertiesLikeJSCS object option is set to true.

What changes did you make? (Give an overview)
Added 2 options to object-property-newline for compatibility with JSCS rule requireObjectKeysOnNewLine.

Is there anything you'd like reviewers to focus on?
Determine whether my judgment is correct that the comma-style rule cannot be relied on to achieve compatibility with the JSCS rule and therefore the noCommaFirst object option is appropriate. See conversation in PR #9522.

[platinumazure]

I've reviewed the documentation and rule implementation, but skipped the tests since I had a number of questions about the first two.

I apologize if I missed some earlier conversation about the direction you've decided to take here, and I appreciate any context you can give me that might help me overcome any misunderstandings I might be exhibiting here. Thanks!

[platinumazure]

Typo: option --> options

[jrpool]

Thank you, @platinumazure, for the review and suggestions. I’ll attend to these.

[platinumazure]

In other rule documentation, we like to use patterns like the below:

Examples of **correct** code with the `allowMultiplePropertiesPerLine: true` option:

```js
/* examples here */
```

Examples of **incorrect** code with the `allowMultiplePropertiesPerLine: true` option:

```js
/* examples here */
```

Referring to other examples ("the first two above") makes the documentation harder to read. It's better to repeat examples than to refer to examples elsewhere in the document.

[platinumazure]

See above comment for info on our example conventions.

[platinumazure]

I think this option name should be more generic and not reference JSCS directly (although the option description can certainly reference JSCS). Maybe something like ignoreBracketsInComputedProperties?

[platinumazure]

I'm not sure I understand this part. Why can't comma-style be used in combination with treatComputedPropertiesLikeJSCS?

Should this be valid or invalid:

/* eslint comma-style: ["error", "first"], object-property-newline: ["error", { treatComputedPropertiesLikeJSCS: true }] */
({
    foo: "bar", [
        "computed"
    ]: true
    , baz: "woo"     // <--- this leading comma here: valid or invalid?
});

[platinumazure]

Nit: I think firstToken or propertyFirstToken would be a better variable name.

[platinumazure]

Nit: I think firstToken or propertyFirstToken would be a better variable name.

[platinumazure]

Why should this be valid? Is that how JSCS handles this case? Would it make more sense to augment comma-style with an option to allow computed properties after commas in "last" mode?

[jrpool]

Yes, JSCS treats such a line as valid per its requireObjectKeysOnNewLine rule. One might presume that if JSCS has such a rule then there is some demand for it.

Given that ESLint wants to establish JSCS compatibility, it seems to me more user-friendly to tell JSCS users how to do that for a single JSCS rule with instructions for configuring a single ESLint rule, rather than making them piece together 2 ESLint rules.

Also, adding an option to comma-style could cover this case but not the case of a line with a lone comma on it. That would need another new option.

Also, this may be due to my inadequate knowledge, but I don’t understand how the comma-style rule can be simultaneously applied oppositely to 2 or more of the 10 node types that it applies to. If my impression is correct, then it seems questionable to load additional options onto a rule that already has so many.

I have added what I think is a thorough explanation of the (in)applicability of comma-style to the noCommaFirst section of docs/rules/object-property-newline. I hope it at least shows why comma-style as it now exists can’t do the job. The other points above lead me to believe that confining these alterations to object-property-newline is, on balance, wiser than giving part of the job to comma-style, but there may be reasons for the latter that I’m not aware of.

[jrpool]

I believe I have completed the responses to all above comments and change requests.

[kaicataldo]

Apologies - it looks like we lost track of this. @eslint/eslint-team Can we give this a look?

[aladdin-add]

@platinumazure have your comments being addressed?

this is a jscs-compatible issue, labelled accepted.

[nzakas]

@platinumazure have your comments been addressed? We should either work to merge this or close it out as this PR is now over a year old.

[kaicataldo]

One more friendly ping here

[platinumazure]

I'm still confused about the special case where, if comma first is not allowed but open bracket could be on its own line, why , [ is valid on its own line. I had suggested that maybe this rule should not try to worry about comma placement and that the comma-style rule should try to handle that case.

I'd have to dig into this again more deeply to try to figure out a more concrete proposal for how this should be handled specifically.

[jrpool]

Thank you, @kaicataldo and @platinumazure, for noticing this long-incomplete thread. There doesn't seem to exist a consensus or decision yet on whether compatibility with the JSCS rule requireObjectKeysOnNewLine may and should require modifications to both object-property-newline and comma-style. Or is there possibly a reason not to insist on 100% compatibility with JSCS?

[platinumazure]

@jrpool My take is that we want to provide a way for any JSCS settings to be achieved (except for any prudent decisions we might make on environment-specific options, etc., which go against ESLint's design principles of trying not to favor libraries and runtimes).

However, I don't think one JSCS rule absolutely needs to map to one ESLint rule. In my comment above, I think my suggestion was we should try to let comma-style and other rules related to commas handle the comma logic, and let this rule focus on newlines around/between object properties. I think we can reasonably note, in rule documentation and/or a hypothetical JSCS migration guide, that for some particular setting, a user might need to configure 2 ESLint rules to get the desired behavior.

That's just my take. @kaicataldo Do you have any thoughts?

[kaicataldo]

Agreed with @platinumazure! My understanding of "parity with JSCS" is that we want to be able to enforce styles that you can in JSCS, but the actual API doesn't have to match.

[ilyavolodin]

Since this PR has been sitting here without any movement for a long while now, I'm going to go ahead and close it. @jrpool thanks again for your effort to push this through!