KT-81448: Support wildcards for power-assert function selection #5519
Conversation
There was a problem hiding this comment.
This looks great. A departure from the glob style mentioned in the ticket, but I'm not strongly opposed to this, personally at least. I'd want some input from the Kotlin Build Tools team (@Tapchicoma) on if this conflicts with any of their design goals, though.
...sert/power-assert.backend/src/org/jetbrains/kotlin/powerassert/PowerAssertCallTransformer.kt
Outdated
Show resolved
Hide resolved
| val project = kotlinCompilation.target.project | ||
| val extension = project.extensions.getByType(PowerAssertGradleExtension::class.java) | ||
| return extension.functions.map { functions -> | ||
| return extension.functions.zip(extension.functionRegexes) { functions, patterns -> |
There was a problem hiding this comment.
This confused me for so long because I thought this was zipping the sets together, rather than zipping the properties together. 😆
Maybe each property could be mapped individually and then zipped at the end? Probably less confusing but also less efficient. Though maybe efficiency isn't super important here? Or maybe there's another way to make it more clear and equally efficient?
There was a problem hiding this comment.
I've pulled out the element mapping to local variables and then ziped at the end. I think it's a bit clearer.
...sert/src/common/kotlin/org/jetbrains/kotlin/powerassert/gradle/PowerAssertGradleExtension.kt
Outdated
Show resolved
Hide resolved
| // Java's Pattern is used here instead of Kotlin's Regex for Groovy compatability | ||
| val functionRegexes: SetProperty<Pattern> = objectFactory.setProperty(Pattern::class.java).convention(setOf()) |
There was a problem hiding this comment.
Hmm... I think it might just be better to make this a Set of Strings. Pattern might logically be correct, but also seems a little too specific. Maybe @Tapchicoma has more specific thoughts here.
There was a problem hiding this comment.
I can see why you might want to use Stirng. There's two primary reasons I went with pattern:
- you get the nice Regex syntax highlighting in
Pattern.compile("...")(IntelliJ's language annotation can't be applied to types, and applying it to the property doesn't work). This is particularly useful when escaping.s in package names. - so that regex flags can be set.
plugins/power-assert/testData/codegen/regex/junitAssertionsRegex.kt
Outdated
Show resolved
Hide resolved
| val flags = value.substringBefore(':', "").toIntOrNull() ?: 0 | ||
| val pattern = value.substringAfter(':') | ||
| configuration.add(KEY_FUNCTION_REGEXES, Pattern.compile(pattern, flags).toRegex()) |
There was a problem hiding this comment.
This flags part is interesting but may expose too many implementation details. Ties in with the comment about not using Pattern as the Gradle property, but rather just a String. Is there a reason to expose this functionality or just a side-effect of using Pattern?
There was a problem hiding this comment.
It was mostly because we're using pattern on both ends anyways, and this is necessary to get an equal pattern on the other end. But looking over the flags, there aren't any that I think would be useful here, so I'm not opposed to dropping it.
.../power-assert.cli/src/org/jetbrains/kotlin/powerassert/PowerAssertCompilerPluginRegistrar.kt
Outdated
Show resolved
Hide resolved
2 participants
Fixes KT-81448, adding wildcard support using regexes.