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

ReturnCount: Make configuration parameter more explicit #5062

Merged
merged 2 commits into from Jul 11, 2022

Conversation

TWiStErRob
Copy link
Member

@TWiStErRob TWiStErRob commented Jul 11, 2022

Background

I want to configure ReturnCount to ignore Android's onMenuItemSelected and onOptionsItemSelected because they should have many returns to prevent bugs and strange behavior.

Problem

In the JSON-schema we have this definition:

            "excludedFunctions": {
              "type": "string"
            },

Stringly typed items are hard to use, because they don't define structure.
It's not clear if this string is a regex, or a pipe separated list, or a comma separated list, or even a semi-colon separated regex list.

Investigation

Note: this investigation has to be done by anyone who wants to use this configuration, and they might not know how to navigate the code or not have will.

Docs

https://detekt.dev/docs/rules/style/#returncount

No conclusive result.

Source code

@Configuration("define functions to be ignored by this check")
private val excludedFunctions: SplitPattern by config("equals") { SplitPattern(it) }

Now I know it's split somehow.

Tests

Now I know it's split by comma.

Deeper look

private fun shouldBeIgnored(function: KtNamedFunction) = excludedFunctions.contains(function.name)

calls

fun contains(value: String?): Boolean = excludes.any { value?.contains(it, ignoreCase = true) == true }

also

fun String.commaSeparatedPattern(vararg delimiters: String = arrayOf(",")): Sequence<String> {
return this
.splitToSequence(*delimiters)
.filter { it.isNotBlank() }
.map { it.trim() }
}

Now I know it's definitely comma separated (from constructor default parameter), and that it's case insensitive (which is strange in a strongly typed case sensitive language), I also know the comma separated list is free-from because of trim and resilient in case there are multiple or trailing commas.

Solution

Update docs to reflect this.

Note

Obviously a more explicit way of doing this would be ideal:

    excludedFunctions:
      # Ignore MenuProvider.onMenuItemSelected,
      # because it has a special structure for bug prevention.
      - onMenuItemSelected
      # Ignore Activity.onOptionsItemSelected and Fragment.onOptionsItemSelected,
      # because they has a special structure for bug prevention.
      - onOptionsItemSelected

but for now, this will do:

    excludedFunctions:
      # Ignore MenuProvider.onMenuItemSelected, Activity.onOptionsItemSelected,
      # and Fragment.onOptionsItemSelected, because they have a special structure for bug prevention.
      '
        onMenuItemSelected,
        onOptionsItemSelected,
      '

@github-actions github-actions bot added the rules label Jul 11, 2022
@@ -59,7 +59,7 @@ class ReturnCount(config: Config = Config.empty) : Rule(config) {
@Configuration("define the maximum number of return statements allowed per function")
private val max: Int by config(2)

@Configuration("define functions to be ignored by this check")
@Configuration("define a free-form comma separated list of function names to be ignored by this check")
Copy link
Member Author

Choose a reason for hiding this comment

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

re "free-form": https://en.wikipedia.org/wiki/Free-form_language

In computer programming, a free-form language is a programming language in which the positioning of characters on the page in program text is insignificant. Program text does not need to be placed in specific columns as on old punched card systems, and frequently ends of lines are insignificant.

Copy link
Member

Choose a reason for hiding this comment

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

Interensting... this shouldn't be a String this should be a List<String>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Different issue ;) in here I just wanted to focus on improving the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

But on a related note: ForbiddenImport has the same problem, but there it's on the happy path, not an exceptional setup.

Copy link
Member

Choose a reason for hiding this comment

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

Why do you say that ForbiddenImport has that issue? It shouldn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

ForbiddenImport was changed in v1.18.0-RC1 by 8e1328a, and we're on 1.18.1 and it still uses a single string, and it works, but anyway, I see in the JSON schema it's now an array and also it's history now :)

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