IGN-10612 WIP refactor jar entry deduplication #56
Conversation
add test for jar presence, duplication, and sort order linting
| "DG" to "modlApi 'commons-dbcp:commons-dbcp:1.4'", | ||
| "CD" to "modlApi 'args4j:args4j:2.0.8'", | ||
| "CDG" to "modlApi 'com.inductiveautomation.ignition:ia-gson:2.10.1'", | ||
| ) |
There was a problem hiding this comment.
This is a nonsense collection of deps just to get more coverage and push the dedup logic to the max.
| """<jar scope="D">designer-0.0.1-SNAPSHOT.jar</jar>""", | ||
| """<jar scope="D">duckdb_jdbc-0.9.2.jar</jar>""", | ||
| """<jar scope="G">JLargeArrays-1.5.jar</jar>""", | ||
| """<jar scope="G">commons-math3-3.5.jar</jar>""", |
| """<jar scope="CD">client-0.0.1-SNAPSHOT.jar</jar>""", | ||
| """<jar scope="CD">jline-2.12.jar</jar>""", | ||
| """<jar scope="DG">commons-dbcp-1.4.jar</jar>""", | ||
| """<jar scope="DG">commons-pool-1.5.4.jar</jar>""", |
| """<jar scope="CDG">javassist-3.12.1.GA.jar</jar>""", | ||
| """<jar scope="CD">args4j-2.0.8.jar</jar>""", | ||
| """<jar scope="CD">client-0.0.1-SNAPSHOT.jar</jar>""", | ||
| """<jar scope="CD">jline-2.12.jar</jar>""", |
There was a problem hiding this comment.
Note the "promotion" of scope commented in test setup above.
| listOf( | ||
| """<jar scope="CDG">common-0.0.1-SNAPSHOT.jar</jar>""", | ||
| """<jar scope="CDG">ia-gson-2.10.1.jar</jar>""", | ||
| """<jar scope="CDG">javassist-3.12.1.GA.jar</jar>""", |
There was a problem hiding this comment.
Note "promotion" of scope commented in test setup above.
| manifests().forEach { (jarName, scope) -> | ||
| "jar" { | ||
| attribute("scope", scope) | ||
| -jarName |
There was a problem hiding this comment.
This method was already metastasizing with transform logic prior to the current PRs so let's stop it doing that and just leave it to writing XML; pushing the transform upstream ...
| @@ -220,12 +215,32 @@ open class WriteModuleXml @Inject constructor(_objects: ObjectFactory) : Default | |||
| return false | |||
| } | |||
|
|
|||
| private fun manifests(): List<ArtifactManifest> { | |||
There was a problem hiding this comment.
Since it's private and used only from one place, no harm in changing the return type.
| }.let { manifests -> | ||
| manifests | ||
| .flatMap { mani -> mani.artifacts } | ||
| .groupBy { arti -> arti.jarName } |
There was a problem hiding this comment.
Explicit args clarify things a little here without having to fire up a type-aware editor.
| ) | ||
| }.joinToString("") | ||
| ) | ||
| }.sortedWith( |
There was a problem hiding this comment.
The above is bulk of the refinement, minimizing the number of intermediate steps of the transform. It seems functionally equivalent to the original PRs given the unit test coverage. All of WriteModuleXmlTest passes.
There was a problem hiding this comment.
Also, all my listening to nerdy array language podcasts is finally paying off. I'll never touch APL, J, Julia, K, R, etc. but some of the mathematical underpinnings are resonating in in the JVM APIs.
| artifactManifestFromJson(manifest.asFile.readText(Charsets.UTF_8)) | ||
| }.let { manifests -> |
There was a problem hiding this comment.
Avoids having to build this collection twice, for second inner use on #232 - #234.
|
Just to link all the things: #52. |
|
Looks good! It was a good idea to add some tests with transitive dependencies. |
|
Shutting this down since it's a Draft, just a code sharing mechanism. |
Do not merge. Just for sharing code.
A stab at "code golf" and clarifying some existing and some new code from #53. So maybe it's code golf + code ping pong.
See my diff comments for reasoning.