Add allowOmitUnit to rule LibraryCodeMustSpecifyReturnType
#5861
Conversation
| @ParameterizedTest( | ||
| name = "does not report for implicit type Unit expression when allowOmitUnit is {0}", | ||
| ) | ||
| @ValueSource(booleans = [true, false]) | ||
| fun `does not report for Unit expression`(allowOmitUnit: Boolean) { | ||
| val code = """ | ||
| fun foo() = Unit | ||
| """.trimIndent() | ||
| val subject = ExplicitlyDefineReturn( | ||
| TestConfig( | ||
| ALLOW_OMIT_UNIT to allowOmitUnit | ||
| ) | ||
| ) | ||
| val findings = subject.compileAndLintWithContext(env, code) | ||
| assertThat(findings).isEmpty() | ||
| } |
There was a problem hiding this comment.
Shouldn't this case be reported unit is not allowed?
There was a problem hiding this comment.
I carried this from previous issues reported at #4004
Codecov Report
@@ Coverage Diff @@
## main #5861 +/- ##
============================================
+ Coverage 84.55% 84.66% +0.11%
- Complexity 3786 3836 +50
============================================
Files 546 549 +3
Lines 12933 13065 +132
Branches 2274 2305 +31
============================================
+ Hits 10935 11061 +126
- Misses 863 868 +5
- Partials 1135 1136 +1
... and 29 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. |
| * | ||
| */ | ||
| @RequiresTypeResolution | ||
| class ExplicitlyDefineReturn(config: Config = Config.empty) : Rule(config) { |
There was a problem hiding this comment.
How is this rule different from LibraryCodeMustSpecifyReturnType?
There was a problem hiding this comment.
Hi @cortinico, only one diff that I could find out(in the current impl of LibraryCodeMustSpecifyReturnType) is that it only checks for public API.
If we can add allowOmitUnit and remove Library from the rule the it will be more general. cc @BraisGabin for his inputs as well
There was a problem hiding this comment.
If we can add allowOmitUnit
Totally
and remove Library from the rule the it will be more general. cc @BraisGabin for his inputs as well
Maybe here we can add an alias?
There was a problem hiding this comment.
Good call @cortinico. I didn't know that we have this already implemented.
I'm ok about adding allowOmitUnit there. But I'm not sure if it's worth to rename the rule at all. That would force the current users to rename it from their config file. And the improvement is not big enought.
|
What's the agreement here? |
|
Hi @cortinico we(me and @BraisGabin) decided to add |
Hi @cortinico / @BraisGabin I have added the |
cortinico
changed the title
allowOmitUnit to rule LibraryCodeMustSpecifyReturnType
Fixes #5692