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
Conversation
@TheSavior, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @rpatil26 and @alberto to be potential reviewers |
Thanks for the pull request, @TheSavior! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
Thanks for the pull request, @TheSavior! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
7dc4e71
to
1cb5910
Compare
LGTM |
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. |
@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. |
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.) |
LGTM, but the issue still needs to be accepted. |
Looks like there are some merge conflicts that need to be resolved now (probably from my bug fix PR - sorry!). |
LGTM |
All fixed |
code: sourceCode.getText(getNonEmptyOperand(node)) | ||
}); | ||
const code = sourceCode.getText(getNonEmptyOperand(node, "")); | ||
|
There was a problem hiding this comment.
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?
Just a few small things - thanks for working on this! |
Sorry! Mouse veered a little too far to the left :( |
LGTM |
Thanks for the comments. All fixed. |
LGTM |
LGTM. Thanks for contributing to ESLint! |
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.