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

Add a new rule no-unused-expressions #24

Merged
merged 3 commits into from
Nov 17, 2016
Merged

Add a new rule no-unused-expressions #24

merged 3 commits into from
Nov 17, 2016

Conversation

job13er
Copy link
Contributor

@job13er job13er commented Nov 16, 2016

This project uses semver, please check the scope of this pr:

  • #patch# - backwards-compatible bug fix
  • #minor# - adding functionality in a backwards-compatible manner
  • #major# - incompatible API change

CHANGELOG

  • Added an additional rule: no-unused-expressions which will disallow expect(foo).to.be.true style assertions. See this issue for more info.

@sandersky
Copy link
Contributor

sandersky commented Nov 16, 2016

👍

Approved with PullApprove

@job13er
Copy link
Contributor Author

job13er commented Nov 16, 2016

👍

Approved with PullApprove

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 5b47440 on job13er:add-rule into * on ciena-frost:master*.

@juwara0
Copy link
Contributor

juwara0 commented Nov 16, 2016

We should turn on the ternary operators in your expressions flag: http://eslint.org/docs/rules/no-unused-expressions#options

@job13er
Copy link
Contributor Author

job13er commented Nov 17, 2016

We should turn on the ternary operators in your expressions flag: http://eslint.org/docs/rules/no-unused-expressions#options

Should we? I wasn't under the impression that this would prevent desirable use of ternary operators, only those where you don't actually assign the value to something. I don't know that we want to encourage the use of ternary operators over if statements if all you're doing is executing something.

Do we really want to see

(shouldItBeDone) ? doIt() : doNotDoIt()

instead of

if (shouldItBeDone) {
  doIt()
} else {
  doNotDoIt()
}

???

EDIT: I just confirmed that the rule allows normal use of ternary operators w/o a problem, it only complains if it's an unused expression.

Is the above what you'd like to be able to do, @juwara0?

@job13er
Copy link
Contributor Author

job13er commented Nov 17, 2016

OK, fine, someone else seems to like that too. While I personally don't like it at all I'll concede that it's not horrible, so I'll add the flag.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f07085a on job13er:add-rule into * on ciena-frost:master*.

@job13er job13er merged commit 1dab2c9 into ciena-frost:master Nov 17, 2016
@job13er job13er deleted the add-rule branch November 17, 2016 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants