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

Remove hardcoded default values from rules #2597

Closed
BraisGabin opened this issue Apr 12, 2020 · 6 comments · Fixed by #3171
Closed

Remove hardcoded default values from rules #2597

BraisGabin opened this issue Apr 12, 2020 · 6 comments · Fixed by #3171
Milestone

Comments

@BraisGabin
Copy link
Member

Expected Behavior

All the lines of code that we have in our production code are "meaningful".

Observed Behavior

The second parameter in this functions is never used in production: valueOrDefault(KEY, "value") and it's really missleading when you are writing/editing a rule.

Context

Working on #2498 I found some rules that doesn't have the default value hardcoded in it. And I created commits to fix that (4073097 and e7c0190)

But as I continue working in #2498 I see that there are even more rules that have this "issue".

I think that those values are there to make the tests easier. But we should not add any code in production to make the tests easier to write.

My proposal: change the use of valueOrDefault(key, value) to valueString(key), valueBoolean(key)...

Good thing: This is not a breaking change.

@arturbosch
Copy link
Member

arturbosch commented Apr 19, 2020

Those values are the same as in default-detekt-config.
If we remove them we need to change the hard dependency on the default-detekt-config or else a programmatically created Rule wouldn't be the same as loaded with --build-upon-default-config.
Do I miss something?

@BraisGabin
Copy link
Member Author

So, for what you are saying, this is wrong, right:

https://github.com/arturbosch/detekt/blob/c5aa152cedb5bf105c2757dda8f4f5e5cf34c405/detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/exceptions/SwallowedException.kt#L66-L77

We should have something like this:

private val ignoredExceptionTypes = SplitPattern(valueOrDefault(IGNORED_EXCEPTION_TYPES,  
  "InterruptedException,NumberFormatException,ParseException,MalformedURLException"))

I was confused here because I saw a lot of rules that have this problem. So I didn't know if the default value could be removed or we should fix the rules that doesn't have the proper default values.

@arturbosch
Copy link
Member

Ah, yes good find. Indeed these properties need the defaults from the configuration documentation.
I would classify this as a bug as users without a config file would not get any issues from these rules.

@juliabirkett
Copy link
Contributor

@arturbosch can you share this config documentation so I can take a look at the expected default values?

@BraisGabin
Copy link
Member Author

I think that artur refers about this documentation: https://detekt.github.io/detekt/comments.html

@juliabirkett
Copy link
Contributor

guys, I just opened my very first PR contributing to Open Source. if you please can take a look and see if it solves this issue 👍

@arturbosch arturbosch added this to the 1.15.0 milestone Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants