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

Also check USELESS_ELVIS in UnreachableCode #6624

Merged
merged 3 commits into from Jan 3, 2024

Conversation

atulgpt
Copy link
Contributor

@atulgpt atulgpt commented Nov 13, 2023

Fixes #6623

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (174cf03) 85.13% compared to head (a3ab768) 85.13%.
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #6624   +/-   ##
=========================================
  Coverage     85.13%   85.13%           
- Complexity     4079     4081    +2     
=========================================
  Files           570      570           
  Lines         13363    13364    +1     
  Branches       2402     2402           
=========================================
+ Hits          11376    11377    +1     
  Misses          791      791           
  Partials       1196     1196           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@3flex
Copy link
Member

3flex commented Nov 17, 2023

I understand why this is added to the existing unreachable code rule but I believe this should be implemented as a separate rule since it's a wrapper of an existing compiler warning. I don't think wrapped compiler warnings should be consolidated in the same rule.

And very technically speaking the Elvis operator itself is not unreachable - it will always be hit.

@BraisGabin
Copy link
Member

I understand why this is added to the existing unreachable code rule but I believe this should be implemented as a separate rule since it's a wrapper of an existing compiler warning. I don't think wrapped compiler warnings should be consolidated in the same rule.

I'm not sure about this. It's true, it's easy to think "two different rules" looking at the implementation. But, as a user, both are unreachable code so I think that it is ok to merge them. A new rule will help to make this more configurable but I think that we don't need such configuration.

@BraisGabin BraisGabin added this to the 2.0.0 milestone Nov 26, 2023
@BraisGabin BraisGabin added pick request Marker for PRs that should be ported to the 1.0 release branch and removed gradle-plugin labels Nov 26, 2023
@BraisGabin BraisGabin merged commit 3e946a4 into detekt:main Jan 3, 2024
23 checks passed
@atulgpt atulgpt deleted the fixes/6623/report-useless-elvis branch January 6, 2024 13:38
cortinico pushed a commit that referenced this pull request Jan 31, 2024
* Also check `USELESS_ELVIS` in `UnreachableCode`

* Remove unnecessary `?: return` from TC code

* Use `||` instead of list and `in`
cortinico pushed a commit that referenced this pull request Jan 31, 2024
* Also check `USELESS_ELVIS` in `UnreachableCode`

* Remove unnecessary `?: return` from TC code

* Use `||` instead of list and `in`
mgroth0 pushed a commit to mgroth0/detekt that referenced this pull request Feb 11, 2024
* Also check `USELESS_ELVIS` in `UnreachableCode`

* Remove unnecessary `?: return` from TC code

* Use `||` instead of list and `in`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pick request Marker for PRs that should be ported to the 1.0 release branch rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False Negative: Useless elvis is not reported by the UnreachableCode
5 participants