Skip to content
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

Fix: Don't require commas after rest properties (fixes #7297) #7298

Merged
merged 1 commit into from Oct 10, 2016

Conversation

not-an-aardvark
Copy link
Member

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

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

See #7297

Please check each item to ensure your pull request is ready:

  • I've read the pull request guide
  • I've included tests for my change
  • (n/a) I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)

This fixes comma-dangle to not require trailing commas after rest properties with experimentalObjectRestSpread enabled, since this is a SyntaxError according to the spec.

Is there anything you'd like reviewers to focus on?

Nothing in particular.

@not-an-aardvark not-an-aardvark added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion labels Oct 4, 2016
@mention-bot
Copy link

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @alberto and @gimenete to be potential reviewers.

@eslintbot
Copy link

LGTM

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

return node.type !== "ArrayPattern" || lastItem.type !== "RestElement";
return !(
(node.type === "ArrayPattern" && lastItem.type === "RestElement") ||
(node.type === "ObjectPattern" && (lastItem.type === "RestProperty" || lastItem.type === "ExperimentalRestProperty"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have nodes with the "RestProperty" type in Espree? Or is this for the benefit of other parsers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen any RestProperty nodes. I put it there assuming that the name will eventually be RestProperty rather than ExperimentalRestProperty once it's no longer experimental.

Copy link
Member

@kaicataldo kaicataldo Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to hear others' opinions on this. My preference would be not to do this "just in case" and instead to add it in when that becomes a reality (or, hopefully, remove the support for "ExperimentalRestProperty" and add in "RestProperty"). If this helps with other parsers that support this syntax (since we do, even though it's still stage 3), then I'm not opposed to it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incidentally, why does espree add the Experimental prefix? I can understand the need to mark it as experimental, but I would think that is already accomplished by naming the option experimentalObjectRestSpread. When the node name eventually does change, it will almost certainly break downstream users, and that seems unnecessary to me. (Each node can only have one type, so this isn't like CSS vendor prefixes where the experimental and final versions can be supported simultaneously.)

Copy link
Member

@hzoo hzoo Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RestProperty is used in Babel and it was added to estree as experimental/ as well estree/estree@6e4d212

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Espree uses ExperimentalRestProperty because the feature was experimental. If there was a change before the proposal was fully added to the specification (which just happened), we had the ability to ensure that rules could target both the old representation and the official versions (RestProperty). I didn't want people relying on an experimental node without knowing that they were doing so.

When we fully implement the spec, we'll use RestProperty.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nzakas nzakas merged commit 52da71e into master Oct 10, 2016
@not-an-aardvark not-an-aardvark deleted the comma-dangle-rest-property branch October 10, 2016 21:59
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants