-
-
Notifications
You must be signed in to change notification settings - Fork 777
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
Allow regex configuration support for Forbidden Import rule #1963
Allow regex configuration support for Forbidden Import rule #1963
Conversation
@@ -21,6 +21,8 @@ import org.jetbrains.kotlin.psi.KtImportDirective | |||
* </noncompliant> | |||
* | |||
* @configuration imports - imports which should not be used (default: `''`) | |||
* @configuration forbiddenPatterns - reports imports which match the specified regular expression. | |||
For example `net.*R`. (default: `""`) |
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.
This should probably not be on a new line
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.
Done !
43b5bfc
to
a5cebcc
Compare
a5cebcc
to
d6943e0
Compare
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.
Thanks for picking up this topic! This really improves this rule.
Please see my comments and only use force push if it’s really necessary.
This makes it easier for us to review the changes you made. Thanks!
@@ -53,7 +56,12 @@ class ForbiddenImport(config: Config = Config.empty) : Rule(config) { | |||
} | |||
} | |||
|
|||
private fun containsForbiddenPattern(import: String): Boolean { | |||
return forbiddenPatterns.pattern.isNotEmpty() && forbiddenPatterns.containsMatchIn(import) |
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.
You could use the shorthand syntax for this function.
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.
Done !
} | ||
|
||
it("should report import when it matches any one of the pattern") { | ||
val findings = |
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.
You could remove this test, because essentially we test the regex engine here. One test that asserts the positive case is enough.
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.
Done !
This is awesome. Thanks @sowmyav24 |
Fixes #1430
forbiddenPatterns
to specify forbidden regex for imports.