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-cond-assign work for ternaries (fixes #10091) #10109

Merged
merged 2 commits into from Mar 22, 2018

Conversation

Projects
None yet
5 participants
@goodoldneon
Contributor

goodoldneon commented Mar 21, 2018

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

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

Addresses issue #10091

What changes did you make? (Give an overview)
Made the no-cond-assign rule also check for ternaries.

@jsf-clabot

This comment has been minimized.

jsf-clabot commented Mar 21, 2018

CLA assistant check
All committers have signed the CLA.

@eslint eslint bot added the triage label Mar 21, 2018

@platinumazure

Code/tests LGTM.

Could you please change the commit message so it begins with "Update:"? We use "Update" for bugfixes that introduce more warnings, to force the change to be semver-minor as a small concession to users who need to make changes for bugfixes that cause more warnings. Thanks!

@not-an-aardvark not-an-aardvark added accepted and removed evaluating labels Mar 21, 2018

@goodoldneon goodoldneon changed the title from Fix: Make no-cond-assign work for ternaries (fixes #10091) to Update: Make no-cond-assign work for ternaries (fixes #10091) Mar 21, 2018

@goodoldneon

This comment has been minimized.

Contributor

goodoldneon commented Mar 21, 2018

How do I change the commit message? Do I amend my commit, then force push? Or will that screw up the pull request? Sorry for the dumb question. This is my first pull request.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 21, 2018

@goodoldneon

This comment has been minimized.

Contributor

goodoldneon commented Mar 21, 2018

I also forgot to add a valid test. Should I add the following line to the valid array, and then make a new commit?

"var x; var b = (x === 0) ? 1 : 0;"

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 21, 2018

@goodoldneon

This comment has been minimized.

Contributor

goodoldneon commented Mar 21, 2018

Pushed a new commit. Is everything OK on my end?

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 21, 2018

I think so, I'll review properly later tonight. Thanks for your flexibility!

@platinumazure

LGTM, thanks for contributing! Waiting for another review from the team before merging.

@Aladdin-ADD

LGTM. thanks!

@platinumazure platinumazure merged commit ea6fb17 into eslint:master Mar 22, 2018

5 checks passed

commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details

@eslint eslint bot locked and limited conversation to collaborators Sep 19, 2018

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