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

[OptionalUnit] Allow a function to declare a Unit return type when it uses a generic function initializer #4371

Merged

Conversation

severn-everett
Copy link
Contributor

This addresses issue #4363.

@@ -9,6 +9,7 @@ import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.rules.isOverride
import org.jetbrains.kotlin.cfg.WhenChecker
import org.jetbrains.kotlin.js.translate.callTranslator.getReturnType
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea why this is in a JavaScript package, but it appears to work in JVM code.

@codecov
Copy link

codecov bot commented Dec 12, 2021

Codecov Report

Merging #4371 (cc5c1f2) into main (92433ea) will increase coverage by 0.02%.
The diff coverage is 70.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4371      +/-   ##
============================================
+ Coverage     84.26%   84.29%   +0.02%     
- Complexity     3263     3275      +12     
============================================
  Files           473      473              
  Lines         10336    10340       +4     
  Branches       1827     1830       +3     
============================================
+ Hits           8710     8716       +6     
  Misses          667      667              
+ Partials        959      957       -2     
Impacted Files Coverage Δ
...rbosch/detekt/rules/style/optional/OptionalUnit.kt 85.96% <70.00%> (+4.83%) ⬆️

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 92433ea...cc5c1f2. Read the comment docs.

@severn-everett
Copy link
Contributor Author

severn-everett commented Dec 13, 2021

@BraisGabin I'm not sure what I'm supposed to do here. CodeCov is requiring that I write a test where one of the two lines below returns a null value:

        val isGenericType = getResolvedCall(bindingContext)?.getReturnType()?.isTypeParameter() == true
        val isNothingType = getType(bindingContext)?.isNothing() == true

Otherwise, the PR is not hitting the target rate of 80% code coverage. The problem is that writing that test would require creating a code snippet where, for example, the function initializer isn't recognized (e.g. fun foo(): Unit = unknownFun { }). However, doing that will fail the compile-test-snippets step, because as the function initializer isn't recognized, the code wouldn't compile IRL. Any ideas on how to write a credible test here that will satisfy both codecov/patch and Pre Merge Checks / compile-test-snippets?

@BraisGabin
Copy link
Member

@BraisGabin I'm not sure what I'm supposed to do here. CodeCov is requiring that I write a test where one of the two lines below returns a null value:

        val isGenericType = getResolvedCall(bindingContext)?.getReturnType()?.isTypeParameter() == true
        val isNothingType = getType(bindingContext)?.isNothing() == true

Otherwise, the PR is not hitting the target rate of 80% code coverage. The problem is that writing that test would require creating a code snippet where, for example, the function initializer isn't recognized (e.g. fun foo(): Unit = unknownFun { }). However, doing that will fail the compile-test-snippets step, because as the function initializer isn't recognized, the code wouldn't compile IRL. Any ideas on how to write a credible test here that will satisfy both codecov/patch and Pre Merge Checks / compile-test-snippets?

Don't care about that. If the tests really cover the main logic it's enough. If you check you are under the target but even with that you are increasing the coverage.

@BraisGabin BraisGabin merged commit b013465 into detekt:main Dec 15, 2021
@BraisGabin BraisGabin added this to the 1.20.0 milestone Dec 15, 2021
@severn-everett severn-everett deleted the allow_generic_initializer_return branch December 16, 2021 07:43
@cortinico cortinico changed the title Allow a function to declare a Unit return type when it uses a generic function initializer [OptionalUnit] Allow a function to declare a Unit return type when it uses a generic function initializer Dec 17, 2021
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.

None yet

3 participants