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

Update: make no-implicit-coercion support autofixing. (fixes #7056) #7061

Merged
merged 3 commits into from Sep 8, 2016

Conversation

TheSavior
Copy link
Contributor

What issue does this pull request address?
#7056

What changes did you make? (Give an overview)
This rule already specifies the fixes to make, this change just makes the code implement those fixes.

Is there anything you'd like reviewers to focus on?
Nothing in particular.

@mention-bot
Copy link

@TheSavior, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @rpatil26 and @alberto to be potential reviewers

@eslintbot
Copy link

Thanks for the pull request, @TheSavior! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.
  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@eslintbot
Copy link

Thanks for the pull request, @TheSavior! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.
  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@TheSavior TheSavior changed the title Add autofix to no-implicit-coercion Update: make no-implicit-coercion support autofixing. (fixes #7056) Sep 5, 2016
@eslintbot
Copy link

LGTM

@TheSavior
Copy link
Contributor Author

TheSavior commented Sep 5, 2016

This eslint bot seems confusing now that commits can be squashed on merge. I added a commit with the correct message format but it still doesn't like it. Does it need to be the first commit?

Edit I just squashed my commits to appease it. Seems unnecessary though.

@platinumazure
Copy link
Member

@TheSavior Yeah, it's talking about the first commit. We should probably remove that check from the bot at some point. Sorry for the inconvenience and thanks for taking the time to squash.

@platinumazure
Copy link
Member

Unrelated to this PR, I'd like to see the message start with a capital letter. (Don't implement this, I'll create an issue so the team can discuss.)

LGTM, but would like at least one more set of eyes on this. (In addition, this should probably wait at least 2 days for merge, maybe 3 if we factor in the US holiday.)

@vitorbal
Copy link
Member

vitorbal commented Sep 7, 2016

LGTM, but the issue still needs to be accepted.

@kaicataldo
Copy link
Member

Looks like there are some merge conflicts that need to be resolved now (probably from my bug fix PR - sorry!).

@eslintbot
Copy link

LGTM

@TheSavior
Copy link
Contributor Author

All fixed

code: sourceCode.getText(getNonEmptyOperand(node))
});
const code = sourceCode.getText(getNonEmptyOperand(node, ""));

Copy link
Member

Choose a reason for hiding this comment

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

getNonEmptyOperand only takes one argument (just node). Can we get rid of the second argument? Also, mind getting rid of this extra newline while you're at it?

@kaicataldo
Copy link
Member

Just a few small things - thanks for working on this!

@kaicataldo kaicataldo closed this Sep 8, 2016
@kaicataldo kaicataldo reopened this Sep 8, 2016
@kaicataldo
Copy link
Member

Sorry! Mouse veered a little too far to the left :(

@eslintbot
Copy link

LGTM

@TheSavior
Copy link
Contributor Author

Thanks for the comments. All fixed.

@ghost
Copy link

ghost commented Sep 8, 2016

LGTM

@kaicataldo
Copy link
Member

LGTM. Thanks for contributing to ESLint!

@kaicataldo kaicataldo merged commit 538d258 into eslint:master Sep 8, 2016
@TheSavior TheSavior deleted the fix-conversion branch September 8, 2016 17:08
@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
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants