Skip to content

Commit

Permalink
[#5] Write rules individually (#6)
Browse files Browse the repository at this point in the history 
* [#5] Write rules individually
  • Loading branch information
@asarkar
asarkar authored Jul 1, 2018
1 parent 0d8f107 commit 14fd635
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 117 deletions.
109 changes: 23 additions & 86 deletions src/main/kotlin/Migrator.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import net.sourceforge.pmd.Properties
import net.sourceforge.pmd.Rule
import net.sourceforge.pmd.Ruleset
import org.slf4j.LoggerFactory
import java.util.Objects
import java.util.concurrent.TimeUnit


Expand All @@ -29,12 +28,12 @@ object Migrator {
.toMap()
}

private fun split(rule: String): Pair<String, String>? {
return "(.*\\.xml)(?:/)?(.*)?".toRegex().matchEntire(rule).let {
internal fun split(ref: String): Pair<String, String>? {
return "(.*\\.xml)(?:/)?(.*)?".toRegex().matchEntire(ref).let {
when (it?.groups?.size) {
3 -> it.groups[1]!!.value to it.groups[2]!!.value
else -> {
LOGGER.warn("No match found for rule: $rule")
LOGGER.warn("No match found for rule: $ref")
null
}
}
Expand Down Expand Up @@ -65,7 +64,8 @@ object Migrator {
}

private fun Rule.toRule(): Rule? {
val name = this.ref?.let { split(it) }?.second ?: return this
val name = if (RULE_MAP.containsKey(this.name)) this.name
else this.ref?.let { split(it) }?.second ?: return this

if (this.name != null && this.name != name) {
LOGGER.warn("Rule: {} has been renamed to: {}", this.name, name)
Expand All @@ -75,47 +75,29 @@ object Migrator {
name = name,
ref = "${RULE_MAP[name]}/$name",
properties = this.properties
)
).also {
it.description = description
it.priority = priority
it.language = language
it.minimumLanguageVersion = minimumLanguageVersion
it.maximumLanguageVersion = maximumLanguageVersion
it.since = since
it.message = message
it.externalInfoUrl = externalInfoUrl
it.clazz = clazz
if (isDfa == true) it.isDfa = true
if (isTypeResolution) it.isTypeResolution = true
}
}

private fun Rule.toRules() = async {
val objectFactory = ObjectFactory()
private fun Rule.toRules(): Deferred<List<Rule>> = async {
val outer = this@toRules

if (outer.ref?.isRuleset() == true) {
LOGGER.debug("Found ruleset: {}", outer.ref)

val categoryMap = PMD.ruleset(outer.ref)
.rule
.map {
split(it.ref)?.apply {
if (it.name != null && it.name != second) {
LOGGER.warn("Rule: {} has been renamed to: {}", it.name, second)
}
}
?.let { it.first }
}
.filter { it != null }
.distinct()
.map { it to emptyList<String>() }
.toMap()

val excludeMap = (outer.exclude ?: emptyList())
.map { it.name }
.groupBy { RULE_MAP[it] }

(categoryMap.keys + excludeMap.keys)
.map { it to ((categoryMap[it] ?: emptyList()) + (excludeMap[it] ?: emptyList())) }
.map { (c, ex) ->
if (ex.isNotEmpty()) {
LOGGER.debug("Found exclusions: $ex")
}
createRule(
ref = c,
exclude = ex
.map { objectFactory.createExclude().apply { this.name = it } }
)
}
PMD.ruleset(outer.ref)
.rule.mapNotNull { it.toRule() }
.filter { outer.exclude.none { e -> e.name == it.name } }
} else {
LOGGER.debug("Found rule: {}", outer.ref)
outer.toRule()?.let {
Expand All @@ -124,7 +106,7 @@ object Migrator {
}
}

private fun String.isRuleset() = isNotMigrated() && this.endsWith(".xml")
private fun String.isRuleset(): Boolean = isNotMigrated() && this.endsWith(".xml")

private fun awaitRules(deferred: Deferred<List<Rule>>): List<Rule> {
return runBlocking {
Expand Down Expand Up @@ -155,36 +137,6 @@ object Migrator {
.toList()
.flatMap { it.second }
}
.groupBy { RuleWrapper(it) }
.map {
val ref = it.key.rule.ref
createRule(
ref = ref,
name = if (ref == null) it.key.rule.name else null,
exclude = it.value.flatMap { it.exclude }.distinctBy { it.name },
properties = it.value.flatMap { it.properties?.property ?: emptyList() }
.let {
objectFactory.createProperties().apply {
property.addAll(it)
}
}
).also { rule ->
it.value.firstOrNull { it.name == rule.name }?.also { orig ->
rule.description = orig.description
rule.priority = orig.priority
rule.language = orig.language
rule.minimumLanguageVersion = orig.minimumLanguageVersion
rule.maximumLanguageVersion = orig.maximumLanguageVersion
rule.since = orig.since
rule.message = orig.message
rule.externalInfoUrl = orig.externalInfoUrl
rule.clazz = orig.clazz
rule.isDfa = orig.isDfa
rule.isTypeResolution = orig.isTypeResolution
rule.isDeprecated = orig.isDeprecated
}
}
}
.let {
objectFactory.createRuleset().apply {
rule.addAll(it.sortedWith(compareBy(nullsLast<String>()) { it.ref }))
Expand All @@ -193,19 +145,4 @@ object Migrator {
}
}
}

class RuleWrapper(val rule: Rule) {
private val key = rule.ref ?: rule.name

override fun equals(other: Any?): Boolean {
if (this === other) return true
if (javaClass != other?.javaClass) return false

return key == (other as RuleWrapper).key
}

override fun hashCode(): Int {
return Objects.hashCode(key)
}
}
}
18 changes: 11 additions & 7 deletions src/main/kotlin/RulesetValidator.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,17 @@ object RulesetValidator {
})
}

val ex1 = first.flatMap { it.exclude.map { it.name } }.toSet()
val ex2 = second.flatMap { it.exclude.map { it.name } }
(ex1 to ex2).apply {
assert(first.size == second.size, {
"Old ruleset had ${first.size} exclusions, new one has ${second.size}. Diff: ${first - second}"
})
}
val excludes = first
.flatMap { it.exclude.map { it.name } }
.toSet()
val oldNames = first
.mapNotNull { it.name ?: it.ref?.let { Migrator.split(it) }?.second }
.toSet()
val newNames = second.mapNotNull { it.name }
.toSet()
// excluded rules shouldn't be in new ruleset unless they'd been redefined
val exclusions = excludes.intersect(newNames) - oldNames
assert(exclusions.isEmpty(), { "Found rules that're supposed to be excluded: $exclusions" })
}
}
}
19 changes: 19 additions & 0 deletions src/test/kotlin/MigratorTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,23 @@ class MigratorTest {

RulesetValidator.validate(ruleset, migrated)
}

@Test
fun `should migrate rules individually`() {
val ruleset = unmarshal(Paths.get(javaClass.getResource("/ruleset-issue-5.xml").toURI()))
val migrated = Migrator.migrate(ruleset)

assertNotNull(migrated)
assertEquals(4, migrated.rule.size)

assertEquals(listOf(
"LooseCoupling",
"CouplingBetweenObjects",
"LawOfDemeter",
"LoosePackageCoupling"),
migrated.rule.map { it.name }
)

RulesetValidator.validate(ruleset, migrated)
}
}
37 changes: 13 additions & 24 deletions src/test/kotlin/RulesetVerifier.kt
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.abhijitsarkar.pmd

import net.sourceforge.pmd.Rule
import net.sourceforge.pmd.Ruleset
import kotlin.test.assertEquals
import kotlin.test.assertNotNull
Expand All @@ -12,31 +11,21 @@ object RulesetVerifier {
fun verify(ruleset: Ruleset) {
assertNotNull(ruleset)

assertEquals(11, ruleset.rule.size)
val refMap = ruleset.rule
.groupBy { Migrator.split(it.ref)?.first ?: it.name }
.mapValues { it.value.flatMap { it?.properties?.property ?: emptyList() }.size }
.toMap()

assertEquals(7, refMap.size)

ruleset.apply {
assertRule(findByRef(this, "category/java/bestpractices.xml"))
assertRule(findByRef(this, "category/java/codestyle.xml"), excludeCount = 9)
assertRule(findByRef(this, "category/java/codestyle.xml/LongVariable"), propertyCount = 1)
assertRule(findByRef(this, "category/java/codestyle.xml/ShortVariable"), propertyCount = 1)
assertRule(findByRef(this, "category/java/codestyle.xml/TooManyStaticImports"), propertyCount = 1)
assertRule(findByRef(this, "category/java/design.xml"), excludeCount = 5)
assertRule(findByRef(this, "category/java/documentation.xml"), excludeCount = 1)
assertRule(findByRef(this, "category/java/errorprone.xml"), excludeCount = 5)
assertRule(findByRef(this, "category/java/errorprone.xml/AvoidDuplicateLiterals"), propertyCount = 1)
assertRule(findByRef(this, "category/java/multithreading.xml"))
assertRule(findByRef(this, "category/java/performance.xml"))
assertEquals(0, refMap["category/java/bestpractices.xml"])
assertEquals(3, refMap["category/java/codestyle.xml"])
assertEquals(0, refMap["category/java/design.xml"])
assertEquals(0, refMap["category/java/documentation.xml"])
assertEquals(1, refMap["category/java/errorprone.xml"])
assertEquals(0, refMap["category/java/multithreading.xml"])
assertEquals(0, refMap["category/java/performance.xml"])
}
}

private fun assertRule(rule: Rule?, excludeCount: Int = 0, propertyCount: Int = 0) {
assertNotNull(rule)
assertEquals(excludeCount, rule?.exclude?.size ?: 0)
assertEquals(propertyCount, rule?.properties?.property?.size ?: 0)
}

private fun findByRef(ruleset: Ruleset, ref: String): Rule? {
return ruleset.rule
.find { it.ref == ref }
}
}
10 changes: 10 additions & 0 deletions src/test/resources/ruleset-issue-5.xml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://pmd.sourceforge.net/ruleset/2.0.0"
name="ruleset"
xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 http://pmd.sourceforge.net/ruleset_2_0_0.xsd">
<description>https://github.com/asarkar/pmd-migration-tool/issues/5</description>
<rule ref="rulesets/java/coupling.xml">
<exclude name="ExcessiveImports"/>
</rule>
</ruleset>

0 comments on commit 14fd635

Please sign in to comment.