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

Lazy regex evaluation on Rules #1080

Merged
merged 7 commits into from
Sep 3, 2018

Conversation

pavlospt
Copy link
Contributor

@pavlospt pavlospt commented Sep 2, 2018

This PR resolves the eager Regex evaluation discussed in #1064

  1. Introduces LazyRegex class that evaluates a Regex needed inside a rule, only when it is requested and only once.
  2. Makes use of LazyRegex in LateinitUsage rule.
  3. Adds 2 extra test steps asserting the discussed behaviour.

@arturbosch I would be more than happy to migrate the rest of the Regex usages inside Rules (if any) once you think this is a proper solution!

Added a test verifying the lazy initialization behaviour when disabled
and the proper failure when enabled.
@pavlospt pavlospt changed the title Lazy regex evaluation Lazy regex evaluation on Rules Sep 2, 2018
Copy link
Member

@arturbosch arturbosch left a comment

Choose a reason for hiding this comment

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

Looks good to me implementation-wise!

@@ -0,0 +1,29 @@
package io.gitlab.arturbosch.detekt.api.internal
Copy link
Member

Choose a reason for hiding this comment

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

Lets move this class out from internal as it will be used by detekt-rules and maybe third party extensions.
Please add one sentence that key and default are actually used to retrieve a config property.

it("should not fail when disabled with faulty regex pattern") {
val configValues = mapOf("active" to "false", LateinitUsage.IGNORE_ON_CLASSES_PATTERN to "*Test")
val findings = LateinitUsage(TestConfig(configValues)).lint(code)
assertThat(findings).hasSize(0)
Copy link
Member

Choose a reason for hiding this comment

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

You can use .isEmpty here and above in the old code :)

@pavlospt
Copy link
Contributor Author

pavlospt commented Sep 2, 2018

@arturbosch updated with your feedback :)

@arturbosch
Copy link
Member

Thanks, feel free to add yourself to the readme and create additional PR with the more regex changes :)

@pavlospt
Copy link
Contributor Author

pavlospt commented Sep 2, 2018

@arturbosch Added my name in README.md! You can merge this one and I will follow up with a new one for the rest of the Rules :)

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Thanks for investigating and implementing this issue! Great work! :)

@arturbosch arturbosch merged commit 2165019 into detekt:master Sep 3, 2018
@arturbosch arturbosch added this to the RC9 milestone Sep 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants