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: prefer-object-spread duplicated comma (fixes #10512, fixes #10532) #10524

Merged
merged 4 commits into from
Jul 8, 2018

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented Jun 26, 2018

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

[X] Bug fix: fixes #10512, fixes #10532

What changes did you make? (Give an overview)

This PR fixes prefer-object-spread rule to generate valid code in autofix.

This PR is massive because it's hard to handle code correctly in text-based way.
The new way is token-based. Basically, it finds unnecessary tokens and removes those.

  1. remove the callee (Object.assign).
  2. replace the parentheses of the argument list to braces (({, )}).
  3. process arguments:
    • if it's an ObjectExpression node, remove braces (and enclosing parentheses), and remove trailing comma if exists.
    • otherwise, insert ... before the argument.

I tried to make close formatting to the original, but there are some difference in spacing.

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

It passes existing test cases, but tell me if you want to see other cases.

@mysticatea mysticatea 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 Jun 26, 2018
@mysticatea mysticatea changed the title Fix: prefer-object-spread duplicated comma (fixes #10512) Fix: prefer-object-spread duplicated comma (fixes #10512, fixes #10532) Jun 28, 2018
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!

@platinumazure
Copy link
Member

I'd like one more set of eyes on this before merging.

Copy link
Member

@not-an-aardvark not-an-aardvark 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!

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM!

@kaicataldo kaicataldo merged commit b2df738 into master Jul 8, 2018
@kaicataldo kaicataldo deleted the issue10512 branch July 8, 2018 20:43
@kaicataldo
Copy link
Member

Thanks for contributing to ESLint!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jan 5, 2019
@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 Jan 5, 2019
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
4 participants