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 for CanBeNonNullable rule #5714

Merged
merged 5 commits into from Apr 6, 2023

Conversation

atulgpt
Copy link
Contributor

@atulgpt atulgpt commented Jan 20, 2023

Added failing TCs
Fixes #5629

@github-actions github-actions bot added the rules label Jan 20, 2023
@atulgpt atulgpt changed the title Add failing TCs Add failing TCs for CanBeNonNullable rule Jan 20, 2023
@BraisGabin
Copy link
Member

Are these tests related with #5629? The cases are a bit difficult to follow. And I'm not sure if all of them are related with the same issue.

Would it be possible to split them in different PRs? Or all of them target the same flaw?

@atulgpt
Copy link
Contributor Author

atulgpt commented Feb 21, 2023

Hi, @BraisGabin not all follow the same flaw. Some can be grouped and split into little PRs. Is there any provision to add a disabled test in the PR? Or should I completely remove them from here and add in the other PRs?

Also only see the TCs with // Failing comments as only those TCs are failing

@BraisGabin
Copy link
Member

I would prefer to split them up.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

Hi @atulgpt which are the next steps for this PR? Is there somerhing blocking you here? Can I help you some how? I left some comments on the tests but they seem fine.

I must say that a function that only does something when a parameter is null seems like a function that should be flagged too. But probably not by this rule but by other one. But this is offtopic for this PR.

}
}
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
Copy link
Member

Choose a reason for hiding this comment

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

I dont't get what this test does exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name of the TC was wrong. Actually, it should report as all the branches of when handles the non-null case. Here we are checking different types of boolean conditions. I have renamed the TC to does report when parameter is not checked for nullability in boolean conditions

@atulgpt
Copy link
Contributor Author

atulgpt commented Mar 7, 2023

Hi @atulgpt which are the next steps for this PR? Is there somerhing blocking you here? Can I help you some how? I left some comments on the tests but they seem fine.

I must say that a function that only does something when a parameter is null seems like a function that should be flagged too. But probably not by this rule but by other one. But this is offtopic for this PR.

Hi @BraisGabin good vibes of Bali is stopping me from completing this PR 😂. I will complete back after I come back 🙂

@BraisGabin
Copy link
Member

Hi @BraisGabin good vibes of Bali is stopping me from completing this PR 😂. I will complete back after I come back 🙂

Enjoy!!!

@codecov
Copy link

codecov bot commented Mar 25, 2023

Codecov Report

Merging #5714 (f9f8b8b) into main (33d0230) will increase coverage by 0.06%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##               main    #5714      +/-   ##
============================================
+ Coverage     84.57%   84.63%   +0.06%     
- Complexity     3786     3788       +2     
============================================
  Files           546      546              
  Lines         12938    12943       +5     
  Branches       2275     2275              
============================================
+ Hits          10942    10954      +12     
- Misses          862      866       +4     
+ Partials       1134     1123      -11     
Impacted Files Coverage Δ
.../arturbosch/detekt/rules/style/CanBeNonNullable.kt 80.07% <87.50%> (+7.24%) ⬆️

... and 19 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@atulgpt atulgpt marked this pull request as ready for review March 25, 2023 14:56
Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

I left some nit comments but LGTM

@BraisGabin BraisGabin added this to the 1.23.0 milestone Mar 27, 2023
atulgpt and others added 2 commits March 28, 2023 13:56
…/rules/style/CanBeNonNullable.kt

Co-authored-by: Brais Gabín <braisgabin@gmail.com>
@BraisGabin
Copy link
Member

It seems that my code didn't compile 😅. The Github's suggestions feature is great but working without an IDE is difficult.

@atulgpt
Copy link
Contributor Author

atulgpt commented Apr 1, 2023

It seems that my code didn't compile 😅. The Github's suggestions feature is great but working without an IDE is difficult.

Haha true 😅 I have fixed it to make it compliable. Now CI should pass

@cortinico cortinico changed the title Add failing TCs for CanBeNonNullable rule Fix false positive for CanBeNonNullable rule Apr 6, 2023
@cortinico cortinico merged commit 5cdd952 into detekt:main Apr 6, 2023
23 checks passed
@atulgpt atulgpt deleted the fixes-5629 branch April 6, 2023 18:40
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.

CanBeNonNullable false positive
3 participants