New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New: Revise rule object-property-newline, tests, and docs (refs #6251) #9522

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jrpool
Contributor

jrpool commented Oct 26, 2017

This (1) adds 2 object options to the rule for compatibility with JSCS rule
requireObjectKeysOnNewLine, (2) deprecates the existing misleading name of
object option allowMultiplePropertiesPerLine and gives it a name representing
its actual functionality, (3) adds tests to cover array and object values and
leading commas, (4) adds tests for the new object options, (5) revises the
rule documentation for the new object options, and (6) revises the rule
documentation to correct omissions and errors.

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

[X] 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:

#6251

What changes did you make? (Give an overview)
Performed step 3 of proposed plan for handling issue #6251.

Is there anything you'd like reviewers to focus on?
This revision incorporates my previous 2 PRs (refs #9498)(refs #9502). If this revision is approved and merged, then those two earlier PRs become superfluous and closable.
It appeared to be impossible to work on this PR without starting from the state created
by those 2 earlier ones, which had (and have) not yet been merged. Please advise as to
whether the procedure that I followed was correct.

New: Revise rule object-property-newline, tests, and docs (refs #6251)
This (1) adds 2 object options to the rule for compatibility with JSCS rule
requireObjectKeysOnNewLine, (2) deprecates the existing misleading named of
object option allowMultiplePropertiesPerLine and gives it a name representing
its actual functionality, (3) adds tests to cover array and object values and
leading commas, (4) adds tests for the new object options, (5) revises the
rule documentation for the new object options, and (6) revises the rule
documentation to correct omissions and errors. A byproduct of the new object
option noCommaFirst is to permit using this rule to require property specifications
in object literals to be on separate lines but only with comma-last style.
@jrpool

This comment has been minimized.

Show comment
Hide comment
@jrpool

jrpool Oct 28, 2017

Contributor

I have resolved the conflicts and issued the “Commit Merge” command. If I need to do more, please advise. Thanks.

Contributor

jrpool commented Oct 28, 2017

I have resolved the conflicts and issued the “Commit Merge” command. If I need to do more, please advise. Thanks.

@Aladdin-ADD

This comment has been minimized.

Show comment
Hide comment
@Aladdin-ADD

Aladdin-ADD Oct 28, 2017

Member

seems got a new conflict. 😄 @jrpool

Member

Aladdin-ADD commented Oct 28, 2017

seems got a new conflict. 😄 @jrpool

@jrpool

This comment has been minimized.

Show comment
Hide comment
@jrpool

jrpool Oct 28, 2017

Contributor

The 18 conflicts that now appear are the 18 conflicts that appeared earlier, and that I resolved completely. After I did that, the appveyor test is failing and the conflict report has not changed. I do not see any conflict that is new. Thanks for your help, @Aladdin-ADD.

Contributor

jrpool commented Oct 28, 2017

The 18 conflicts that now appear are the 18 conflicts that appeared earlier, and that I resolved completely. After I did that, the appveyor test is failing and the conflict report has not changed. I do not see any conflict that is new. Thanks for your help, @Aladdin-ADD.

@Aladdin-ADD Aladdin-ADD reopened this Oct 28, 2017

@jrpool

This comment has been minimized.

Show comment
Hide comment
@jrpool

jrpool Oct 28, 2017

Contributor

Thank you, @Aladdin-ADD. I see that there is no longer any conflict.

Contributor

jrpool commented Oct 28, 2017

Thank you, @Aladdin-ADD. I see that there is no longer any conflict.

(This behavior differs from that of the JSCS rule cited below, which does not treat the leading `[` of a computed property name as part of that property specification. The JSCS rule prohibits the second of these formats but permits the first.)
Otherwise, this rule will prohibit it, because ESLint treats the opening bracket of a computed property name as part of the property specification. The JSCS rule `requireObjectKeysOnNewLine` does not, so this object option makes ESLint compatible with JSCS in this respect.
#### `noCommaFirst`

This comment has been minimized.

@Aladdin-ADD

Aladdin-ADD Oct 30, 2017

Member

is this option new added?
the rulename object-newline-, so it might not be the rule's responsibility.(and we already have a rule comma-style for this).

@Aladdin-ADD

Aladdin-ADD Oct 30, 2017

Member

is this option new added?
the rulename object-newline-, so it might not be the rule's responsibility.(and we already have a rule comma-style for this).

This comment has been minimized.

@jrpool

jrpool Oct 30, 2017

Contributor

Yes, this option has been implemented and tested. Its implementation and its tests are included in this pull request.

Thank you, @Aladdin-ADD, for calling attention to the comma-style rule. Here are my thoughts about this question:

  1. You are correct that the comma-style rule deals with the same problem as the noCommaFirst object option in this PR. In principle, it would be possible to use the comma-style rule in combination with this object-property-newline rule to emulate the JSCS requireObjectKeysOnNewLine rule.

  2. However, there seem to be good reasons not to do that:

2a. The main purpose of this issue (refs #6251) is to make the ESLint object-property-newline rule compatible with 2 rules of JSCS.

2b. It would be difficult to use comma-style for this purpose. The user would be required to invoke both rules, and, in invoking the comma-style (last) rule, would need to invoke its exceptions object with 9 enumerated exceptions, to exclude all applications of the rule except ObjectExpression.

2c. Even that would not produce full compatibility. The combination of object options noCommaFirst and treatComputedPropertiesLikeJSCS makes the pattern foo: 'bar'\n, [\nfoo + 'key']: 'baz' valid. But the combination of treatComputedPropertiesLikeJSCS with the comma-style (last) rule would make it invalid.

2d. The combination of these two rules would produce stylistically inconsistent error messages. This rule reports line and column numbers, but the comma-style rule does not.

I look forward to your thoughs about these concerns.

@jrpool

jrpool Oct 30, 2017

Contributor

Yes, this option has been implemented and tested. Its implementation and its tests are included in this pull request.

Thank you, @Aladdin-ADD, for calling attention to the comma-style rule. Here are my thoughts about this question:

  1. You are correct that the comma-style rule deals with the same problem as the noCommaFirst object option in this PR. In principle, it would be possible to use the comma-style rule in combination with this object-property-newline rule to emulate the JSCS requireObjectKeysOnNewLine rule.

  2. However, there seem to be good reasons not to do that:

2a. The main purpose of this issue (refs #6251) is to make the ESLint object-property-newline rule compatible with 2 rules of JSCS.

2b. It would be difficult to use comma-style for this purpose. The user would be required to invoke both rules, and, in invoking the comma-style (last) rule, would need to invoke its exceptions object with 9 enumerated exceptions, to exclude all applications of the rule except ObjectExpression.

2c. Even that would not produce full compatibility. The combination of object options noCommaFirst and treatComputedPropertiesLikeJSCS makes the pattern foo: 'bar'\n, [\nfoo + 'key']: 'baz' valid. But the combination of treatComputedPropertiesLikeJSCS with the comma-style (last) rule would make it invalid.

2d. The combination of these two rules would produce stylistically inconsistent error messages. This rule reports line and column numbers, but the comma-style rule does not.

I look forward to your thoughs about these concerns.

parserOptions: { ecmaVersion: 6 },
errors: [
{
message: "Object properties must go on a new line. The opening bracket of a computed property name may end a line on which another property appears. The comma delimiting two properties may not share a line with any of the second property.",

This comment has been minimized.

@Aladdin-ADD

Aladdin-ADD Oct 30, 2017

Member

these messages are a bit too long, can be simple?

@Aladdin-ADD

Aladdin-ADD Oct 30, 2017

Member

these messages are a bit too long, can be simple?

This comment has been minimized.

@jrpool

jrpool Oct 30, 2017

Contributor

Yes, I can shorten them. I’ll do that.

@jrpool

jrpool Oct 30, 2017

Contributor

Yes, I can shorten them. I’ll do that.

@not-an-aardvark

This comment has been minimized.

Show comment
Hide comment
@not-an-aardvark

not-an-aardvark Oct 30, 2017

Member

Thanks for the PR!

If I'm understanding correctly, this PR makes multiple changes which are only loosely related:

  • Deprecates the allowMultiplePropertiesPerLine option in favor of a replacement with a better name
  • Creates the noCommaFirst option
  • Creates the treatComputedPropertiesLikeJSCS option

Would you mind splitting this up into smaller PRs? It's generally easier to discuss and review proposed changes when they're not all bundled into the same PR, because this allows some of the changes to be merged incrementally. If there is disagreement about a particular feature of a large PR, then the entire PR ends up blocked, but if there is disagreement about a feature on one of several smaller PRs, then the other PRs can still get merged while that feature is discussed.

For example, I agree that it might be a good idea to rename the allowMultiplePropertiesPerLine option, but that wasn't accepted as part of the original proposal in #6251. I'm also not opposed to having an option like treatComputedPropertiesLikeJSCS, but I think we should probably come up with a better option name. Understanding the name treatComputedPropertiesLikeJSCS requires users to know what JSCS did in this case, and I suspect very few users would remember this without reading the docs.

I don't want to block renaming allowMultiplePropertiesPerLine over my concern about an unrelated option, so it would be nice if these were part of separate PRs.

Member

not-an-aardvark commented Oct 30, 2017

Thanks for the PR!

If I'm understanding correctly, this PR makes multiple changes which are only loosely related:

  • Deprecates the allowMultiplePropertiesPerLine option in favor of a replacement with a better name
  • Creates the noCommaFirst option
  • Creates the treatComputedPropertiesLikeJSCS option

Would you mind splitting this up into smaller PRs? It's generally easier to discuss and review proposed changes when they're not all bundled into the same PR, because this allows some of the changes to be merged incrementally. If there is disagreement about a particular feature of a large PR, then the entire PR ends up blocked, but if there is disagreement about a feature on one of several smaller PRs, then the other PRs can still get merged while that feature is discussed.

For example, I agree that it might be a good idea to rename the allowMultiplePropertiesPerLine option, but that wasn't accepted as part of the original proposal in #6251. I'm also not opposed to having an option like treatComputedPropertiesLikeJSCS, but I think we should probably come up with a better option name. Understanding the name treatComputedPropertiesLikeJSCS requires users to know what JSCS did in this case, and I suspect very few users would remember this without reading the docs.

I don't want to block renaming allowMultiplePropertiesPerLine over my concern about an unrelated option, so it would be nice if these were part of separate PRs.

@jrpool

This comment has been minimized.

Show comment
Hide comment
@jrpool

jrpool Oct 30, 2017

Contributor

Thanks for the guidance, @not-an-aardvark. Yes, I can do that.

Contributor

jrpool commented Oct 30, 2017

Thanks for the guidance, @not-an-aardvark. Yes, I can do that.

@jrpool

This comment has been minimized.

Show comment
Hide comment
@jrpool

jrpool Oct 30, 2017

Contributor

When I split this into multiple PRs, it seems that I need to submit them one at a time and wait for each to be approved and merged before submitting the next, if they are interdependent. For example, if we deprecate the existing option name, that deprecated name is still used in some tests, to ensure that the deprecated name is property handled, and some of those tests are tests of new options. So I can’t submit a PR for the new options until I know whether the name-deprecation PR is accepted. So, as a general rule, I should wait to work on anything that depends on a submitted PR until I know the outcome of that PR. Is that a correct understanding, @not-an-aardvark ? Thanks.

Contributor

jrpool commented Oct 30, 2017

When I split this into multiple PRs, it seems that I need to submit them one at a time and wait for each to be approved and merged before submitting the next, if they are interdependent. For example, if we deprecate the existing option name, that deprecated name is still used in some tests, to ensure that the deprecated name is property handled, and some of those tests are tests of new options. So I can’t submit a PR for the new options until I know whether the name-deprecation PR is accepted. So, as a general rule, I should wait to work on anything that depends on a submitted PR until I know the outcome of that PR. Is that a correct understanding, @not-an-aardvark ? Thanks.

@not-an-aardvark

This comment has been minimized.

Show comment
Hide comment
@not-an-aardvark

not-an-aardvark Oct 30, 2017

Member

My recommendation would be to create each PR from the current state of the object-property-newline rule, so that they can each be merged independently at any given time. After one PR is merged, you can rebase the other PRs and fix the merge conflicts.

For example, you could create a PR to add new options now, using the allowMultiplePropertiesPerLine option in the tests. Separately, you could create a PR to rename allowMultiplePropertiesPerLine to allowAllPropertiesOnSameLine. Then when one of the PRs gets merged, you could rebase the other PR and update allowAllPropertiesOnSameLine to allowMultiplePropertiesPerLinein the new tests.

Member

not-an-aardvark commented Oct 30, 2017

My recommendation would be to create each PR from the current state of the object-property-newline rule, so that they can each be merged independently at any given time. After one PR is merged, you can rebase the other PRs and fix the merge conflicts.

For example, you could create a PR to add new options now, using the allowMultiplePropertiesPerLine option in the tests. Separately, you could create a PR to rename allowMultiplePropertiesPerLine to allowAllPropertiesOnSameLine. Then when one of the PRs gets merged, you could rebase the other PR and update allowAllPropertiesOnSameLine to allowMultiplePropertiesPerLinein the new tests.

@jrpool

This comment has been minimized.

Show comment
Hide comment
@jrpool

jrpool Oct 30, 2017

Contributor

OK, thanks for this advice. I can proceed accordingly.

Contributor

jrpool commented Oct 30, 2017

OK, thanks for this advice. I can proceed accordingly.

@jrpool

This comment has been minimized.

Show comment
Hide comment
@jrpool

jrpool Oct 30, 2017

Contributor

Should I close this PR in the meantime?

Contributor

jrpool commented Oct 30, 2017

Should I close this PR in the meantime?

@not-an-aardvark

This comment has been minimized.

Show comment
Hide comment
@not-an-aardvark

not-an-aardvark Oct 30, 2017

Member

I have no strong preference either way. I suppose it could be better to close this PR rather than just putting one of the changes on it, since some of the discussion above will no longer be relevant.

Member

not-an-aardvark commented Oct 30, 2017

I have no strong preference either way. I suppose it could be better to close this PR rather than just putting one of the changes on it, since some of the discussion above will no longer be relevant.

@jrpool

This comment has been minimized.

Show comment
Hide comment
@jrpool

jrpool Oct 30, 2017

Contributor

Closing this PR in order to submit multiple simpler PRs covering the same territory.

Contributor

jrpool commented Oct 30, 2017

Closing this PR in order to submit multiple simpler PRs covering the same territory.

@jrpool jrpool closed this Oct 30, 2017

@eslint eslint bot locked and limited conversation to collaborators Apr 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.