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

Implement ExplicitCollectionElementAccessMethod rule #2215

Merged

Conversation

smyachenkov
Copy link
Contributor

This PR adds new rule ExplicitCollectionElementAccessMethod to style ruleset.

Rule ExplicitCollectionElementAccessMethod reports usage of get or put methods and suggests to use Indexed access operator: []. This rule can be applied to Kotlin collections which implement Map or List interfaces as well as Java collections java.util.AbstractMap and java.util.AbstractList.

Noncompliant Code:

val map = mapOf<String, String>() 
val list = listOf<String>() 

map.put("key", "val") 
map.get("key") 
list.get(0) 

Compliant Code:

val map = mapOf<String, String>() 
map["key"] = "val" 
map["key"] 
list[0] 

This is a rule for Style ruleset. It is implemented to introduce users to indexed access operator and help them write shorter and more idiomatic code.

@schalkms
Copy link
Member

schalkms commented Jan 7, 2020

Please rebase since #2217 was closed.

@smyachenkov smyachenkov force-pushed the rule-explicitcollectionelementaccessmethod branch from 50a3667 to 9925c4a Compare January 8, 2020 18:12
@codecov-io
Copy link

codecov-io commented Jan 8, 2020

Codecov Report

Merging #2215 into master will decrease coverage by 0.1%.
The diff coverage is 78.78%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2215      +/-   ##
============================================
- Coverage     81.35%   81.24%   -0.11%     
- Complexity     2067     2111      +44     
============================================
  Files           341      349       +8     
  Lines          5968     6053      +85     
  Branches       1086     1106      +20     
============================================
+ Hits           4855     4918      +63     
- Misses          532      535       +3     
- Partials        581      600      +19
Impacted Files Coverage Δ Complexity Δ
...bosch/detekt/rules/providers/StyleGuideProvider.kt 100% <100%> (ø) 3 <0> (ø) ⬇️
...les/style/ExplicitCollectionElementAccessMethod.kt 78.12% <78.12%> (ø) 10 <10> (?)
.../gitlab/arturbosch/detekt/rules/MethodSignature.kt 50% <0%> (-10%) 0% <0%> (ø)
...ab/arturbosch/detekt/cli/console/FindingsReport.kt 88% <0%> (-6.12%) 7% <0%> (ø)
...in/kotlin/io/gitlab/arturbosch/detekt/api/Issue.kt 85.71% <0%> (-3.76%) 5% <0%> (ø)
...osch/detekt/cli/console/FileBasedFindingsReport.kt 89.28% <0%> (-3.31%) 7% <0%> (-1%)
...bosch/detekt/rules/complexity/MethodOverloading.kt 91.17% <0%> (-2.16%) 5% <0%> (ø)
...turbosch/detekt/rules/style/UnusedPrivateMember.kt 93.47% <0%> (ø) 5% <0%> (ø) ⬇️
...osch/detekt/rules/bugs/WrongEqualsTypeParameter.kt 100% <0%> (ø) 10% <0%> (+3%) ⬆️
...ch/detekt/formatting/wrappers/EnumEntryNameCase.kt 100% <0%> (ø) 2% <0%> (?)
... and 14 more

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 43a0982...ae803d8. Read the comment docs.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this rule. First time Kotlin users will certainly benefit from this rule! 👍
Can we please make sure that the implementation is thoroughly tested? Codecov does report some misses for the rule implementation. After that, the build is completely green and I'll merge this!
Please see my other minor comments about the implementation.

?.text in setOf("get", "put")
}

@Suppress("ReturnCount")
Copy link
Member

Choose a reason for hiding this comment

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

Please try to work around this @Suppress by merging the first two if's for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've refactored this class to avoid usage of @Suppress, please check it one more time

smyachenkov and others added 10 commits January 13, 2020 23:41
…/style/ExplicitCollectionElementAccessMethod.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
…/style/ExplicitCollectionElementAccessMethod.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
…/style/ExplicitCollectionElementAccessMethodSpec.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
…/style/ExplicitCollectionElementAccessMethodSpec.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
…/style/ExplicitCollectionElementAccessMethodSpec.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
…/style/ExplicitCollectionElementAccessMethodSpec.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Thanks that looks good!

…/style/ExplicitCollectionElementAccessMethod.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
@schalkms schalkms merged commit 973953f into detekt:master Jan 14, 2020
@arturbosch arturbosch added this to the 1.5.0 milestone Jan 14, 2020
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.

4 participants