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

Fix false-positive on ExplicitCollectionElementAccessMethod #4400

Merged
merged 1 commit into from Dec 27, 2021

Conversation

BraisGabin
Copy link
Member

This fixes the issue raised con #4288 implementing the solution proposed in this comment: #4288 (comment):

Alternatively, we can report only when the set/get has exactly one parameter for the key.

In this case I'm just checking for the set case because I think that all the cases for get should work as expected.

fixes #4288

@codecov
Copy link

codecov bot commented Dec 26, 2021

Codecov Report

Merging #4400 (0da9400) into main (8a919d9) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

❗ Current head 0da9400 differs from pull request most recent head 45c32ca. Consider uploading reports for the commit 45c32ca to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4400      +/-   ##
============================================
- Coverage     84.35%   84.35%   -0.01%     
- Complexity     3277     3279       +2     
============================================
  Files           473      473              
  Lines         10357    10361       +4     
  Branches       1827     1829       +2     
============================================
+ Hits           8737     8740       +3     
  Misses          668      668              
- Partials        952      953       +1     
Impacted Files Coverage Δ
...les/style/ExplicitCollectionElementAccessMethod.kt 63.63% <50.00%> (+1.56%) ⬆️

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 aef4cf9...45c32ca. Read the comment docs.

@BraisGabin BraisGabin force-pushed the fix-4288 branch 2 times, most recently from cf19942 to 27e9a7a Compare December 26, 2021 20:10
@BraisGabin BraisGabin added this to the 1.20.0 milestone Dec 26, 2021
when {
function == null -> false
!function.isOperator -> false
else -> !(function.isFromJava && function.valueParameters.size > 2)
Copy link
Member

Choose a reason for hiding this comment

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

Here you check for the parameter list size to be bigger than 2, but in the test case underneath you assert the following.

does not report if the function has more than 3 arguments and it's defined in java

What system behavior can the user of this rule expect in the mentioned case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here you check for the parameter list size to be bigger than 2, but in the test case underneath you assert the following.

does not report if the function has more than 3 arguments and it's defined in java

I fixed the description of the test to:

does not report if the function has 3 or more arguments and it's defined in java

What system behavior can the user of this rule expect in the mentioned case?

(I'm not sure if I understood what you are asking) The idea is to ignore the cases where the a function set is called and if it is defined in java and java 3 or more arguments. The full context is in the linked issue.

Copy link
Member

Choose a reason for hiding this comment

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

I'm referring to the logical and numerical difference between the two statements.

code:  args > 2
test:  args > 3

Thanks for fixing the test. However, I think the code snippet in the test should then have exactly 3 parameters to assert the boundary (e.g. 3).
Excuse me for being a bit picky here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@BraisGabin BraisGabin merged commit 73203f3 into main Dec 27, 2021
@BraisGabin BraisGabin deleted the fix-4288 branch December 27, 2021 13:27
@cortinico cortinico added the rules label Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExplicitCollectionElementAccessMethod false positive
3 participants