Dark Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

#154 - Add new Ternary sniff#237

Open
tfrommen wants to merge 3 commits intomasterfrom
ternary-sniff
Open

#154 - Add new Ternary sniff#237
tfrommen wants to merge 3 commits intomasterfrom
ternary-sniff

Conversation

Copy link
Contributor

tfrommen commented Oct 28, 2020

This PR adds a new sniff, HM.PHP.Ternary that adds a warning for unnecessary ternary expression, as mentioned in #154:

  • $expr ? true : false;
  • $expr ? false : true.

See fixtures for (passing and failing) example code.

Output is as follows:

3 | WARNING | Unnecessary ternary found: Instead of "$expr ? true : false", use "(bool) $expr"
4 | WARNING | Unnecessary ternary found: Instead of "$expr ? false : true", use "! $expr"

Can anyone think of any edge-case usage that the sniff would either flag as false-positive, or that it would miss?

I'm not 100% sure I considered all tokens that would end the ternary, which currently are: semicolon, closing parenthesis, closing curly brace (function or any other scope), and also comma (which I just added in a subsequent commit).
Anything else?
Maybe @jrfnl?

tfrommen requested review from fklein-lu, kadamwhite and rmccue October 28, 2020 14:20
tfrommen self-assigned this Oct 28, 2020
Copy link
Contributor Author

tfrommen commented Feb 4, 2021

Instead of a custom sniff, we might want to use the UselessTernaryOperatorSniff one included in the Slevomat Coding Standards...? This one is also fixable, which is nice.

I think I will create a separate issue to discuss pulling in the Slevomat Coding Standards...

roborourke reacted with thumbs up emoji

tfrommen removed their assignment Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

rmccue Awaiting requested review from rmccue

kadamwhite Awaiting requested review from kadamwhite

fklein-lu Awaiting requested review from fklein-lu

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant