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 new rule CouldBeSequence #4855

Merged
merged 3 commits into from Jun 1, 2022
Merged

Add new rule CouldBeSequence #4855

merged 3 commits into from Jun 1, 2022

Conversation

themkat
Copy link
Contributor

@themkat themkat commented May 23, 2022

My attempt at #4838

Some inspiration taken from UnnecessaryFilter, and the isCalling method is stolen from there. (why re-invent the wheel?). A little unsure about the documentation parts, so very happy to hear about possible improvements. Would also love to hear if the code could be done in a more functional style that I might have missed.

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff 🎉 Thanks for sendign this over!

@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #4855 (854d015) into main (7cb5d96) will increase coverage by 0.05%.
The diff coverage is 86.04%.

@@             Coverage Diff              @@
##               main    #4855      +/-   ##
============================================
+ Coverage     84.75%   84.80%   +0.05%     
- Complexity     3431     3477      +46     
============================================
  Files           491      494       +3     
  Lines         11278    11395     +117     
  Branches       2076     2099      +23     
============================================
+ Hits           9559     9664     +105     
- Misses          673      676       +3     
- Partials       1046     1055       +9     
Impacted Files Coverage Δ
...rbosch/detekt/rules/performance/CouldBeSequence.kt 85.36% <85.36%> (ø)
...ch/detekt/rules/performance/PerformanceProvider.kt 100.00% <100.00%> (ø)
...les/style/ExplicitCollectionElementAccessMethod.kt 65.71% <0.00%> (-0.96%) ⬇️
...rbosch/detekt/rules/style/UnnecessaryInnerClass.kt 91.22% <0.00%> (-0.08%) ⬇️
...rturbosch/detekt/rules/style/StyleGuideProvider.kt 100.00% <0.00%> (ø)
...detekt/rules/documentation/CommentSmellProvider.kt 100.00% <0.00%> (ø)
...urbosch/detekt/rules/style/NullableBooleanCheck.kt 83.33% <0.00%> (ø)
...s/documentation/KDocReferencesNonPublicProperty.kt 100.00% <0.00%> (ø)
...etekt/generator/printer/defaultconfig/Exclusion.kt 96.77% <0.00%> (+0.10%) ⬆️
...n/kotlin/io/github/detekt/parser/DetektPomModel.kt 80.00% <0.00%> (+1.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cb5d96...854d015. Read the comment docs.

@mjsztainbok
Copy link

mjsztainbok commented Jun 1, 2022

@BraisGabin I don't think that the rule actually does completely resolve this issue.

It only looks for a subset of the operations that can be chained. The rule should also apply for something like list.map { it.first }.any { it == 5} . In the worst case 2 iterations of the list are done here while with a sequence only one is done. The terminal expressions also need to be considered chained operations.

@BraisGabin
Copy link
Member

I agree with you, I would track that as an improvement or false negative on this rule. Can you define the missing cases in an issue and/or open a PR fixing those?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable changes Marker for notable changes in the changelog rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants