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 KotlinBuiltIns extensions #7376

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add KotlinBuiltIns extensions #7376

wants to merge 1 commit into from

Conversation

3flex
Copy link
Member

@3flex 3flex commented Jun 15, 2024

These would ideally be provided by org.jetbrains.kotlin.types.typeUtil which has wrappers for most KotlinBuiltIns functions. Those that are missing that would be generally useful can be added to detekt's TypeUtils.

@detekt-ci
Copy link
Collaborator

detekt-ci commented Jun 15, 2024

Warnings
⚠️ It looks like this PR contains functional changes without a corresponding test.

Generated by 🚫 dangerJS against 5be07d8

These would ideally be provided by org.jetbrains.kotlin.types.typeUtil
which has wrappers for most KotlinBuiltIns functions. Those that are
missing that would be generally useful can be added to detekt's TypeUtils.
Copy link

codecov bot commented Jun 15, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.51%. Comparing base (cda549c) to head (5be07d8).
Report is 3 commits behind head on main.

Files Patch % Lines
...urbosch/detekt/rules/performance/ArrayPrimitive.kt 0.00% 0 Missing and 1 partial ⚠️
...rbosch/detekt/rules/style/RedundantExplicitType.kt 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7376      +/-   ##
============================================
+ Coverage     84.50%   84.51%   +0.01%     
- Complexity     4168     4169       +1     
============================================
  Files           573      573              
  Lines         11897    11899       +2     
  Branches       2461     2461              
============================================
+ Hits          10053    10056       +3     
  Misses          591      591              
+ Partials       1253     1252       -1     

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

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.

Should we forbid the usages of the KotlinBuildIns? With a message to point to these functions.

@@ -21,6 +22,10 @@ fun KotlinType.fqNameOrNull(): FqName? {
return TypeUtils.getClassDescriptor(this)?.fqNameOrNull()
}

fun KotlinType?.isString(): Boolean = KotlinBuiltIns.isString(this)
Copy link
Member

Choose a reason for hiding this comment

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

In general I'm not a big fan of extensions over nullable. Should null.isString() be true or false? To me that's something that the caller should decide.

Copy link
Member

Choose a reason for hiding this comment

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

+1

@cortinico
Copy link
Member

Should we forbid the usages of the KotlinBuildIns? With a message to point to these functions.

+1 to this 👍

@cortinico
Copy link
Member

@3flex this can be rebase and merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants