-
-
Notifications
You must be signed in to change notification settings - Fork 783
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 ReturnCount false positive when excludeReturnFromLambda is enabled #5459
Conversation
@@ -65,7 +64,7 @@ class ReturnCount(config: Config = Config.empty) : Rule(config) { | |||
@Configuration("if labeled return statements should be ignored") | |||
private val excludeLabeled: Boolean by config(false) | |||
|
|||
@Configuration("if labeled return from a lambda should be ignored") | |||
@Configuration("if labeled return from a lambda should be ignored (takes precedence over excludeLabeled.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Configuration("if labeled return from a lambda should be ignored (takes precedence over excludeLabeled.") | |
@Configuration("if labeled return from a lambda should be ignored (takes precedence over excludeLabeled).") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, the configurations for excluding seem to be or
conditions. (The added explanation is only making more confusion for the user). So I revert it.
.none() | ||
.not() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.none().not() is sort of confusing here. Can we rewrite with any or count() >= 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.count()
will unnecessarily materialize the sequence. .any()
returns a Boolean and will only materialize at most one element of the sequence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be isNotEmpty
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a Sequence, isNotEmpty
is only on Collections, because for a "sequence" emptiness doesn't mathematically make sense: "Unlike collections, sequences don't contain elements, they produce them."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #5341
When excludeReturnFromLambda is enabled, then ignore every labeled return in lambda block.
There can be some extra cases like below, which may go against
excludeReturnFromLambda
's general purpose. (do not count return for lambda)But for a clear definition of the
excludeReturnFromLambda
(if the labeled return from a lambda should be ignored), I excluded it.