From 02832da9eab74f703742e23e158649aad5b1741f Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Tue, 28 Nov 2023 14:31:50 +0100 Subject: [PATCH 1/5] docs(compare-command): Add / improve some code comments Signed-off-by: Sebastian Schuberth --- .../compare/src/main/kotlin/CompareCommand.kt | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/plugins/commands/compare/src/main/kotlin/CompareCommand.kt b/plugins/commands/compare/src/main/kotlin/CompareCommand.kt index 34e51f67b3eef..62701887b8c55 100644 --- a/plugins/commands/compare/src/main/kotlin/CompareCommand.kt +++ b/plugins/commands/compare/src/main/kotlin/CompareCommand.kt @@ -94,9 +94,10 @@ class CompareCommand : OrtCommand( throw ProgramResult(2) } + // Arbitrarily determine the mapper from the first file as the file extensions are ensured to be the same. val deserializer = fileA.mapper().registerModule( SimpleModule().apply { - // TODO: Find a way to also ignore temporary directories. + // TODO: Find a way to also ignore temporary directories (when diffing semantically). if (ignoreTime) addDeserializer(Instant::class.java, EpochInstantDeserializer()) if (ignoreEnvironment) addDeserializer(Environment::class.java, DefaultEnvironmentDeserializer()) } @@ -172,12 +173,18 @@ private enum class CompareMethod { private class EpochInstantDeserializer : StdDeserializer(Instant::class.java) { override fun deserialize(parser: JsonParser, context: DeserializationContext): Instant = - Instant.EPOCH.also { parser.codec.readTree(parser) } + Instant.EPOCH.also { + // Just consume the JSON text node without actually using it. + parser.codec.readTree(parser) + } } private class DefaultEnvironmentDeserializer : StdDeserializer(Environment::class.java) { override fun deserialize(parser: JsonParser, context: DeserializationContext): Environment = - Environment().also { parser.codec.readTree(parser) } + Environment().also { + // Just consume the JSON object node without actually using it. + parser.codec.readTree(parser) + } } private fun Map.replaceIn(text: String) = From e3109bdfa85a7a739d78233a61851997eecd20a9 Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Tue, 28 Nov 2023 14:33:01 +0100 Subject: [PATCH 2/5] refactor(compare-command): Move an `enum classs` closer to its use While the `enum class` does not need command context, a nested class in Kotlin does not have outer class context anyway (only `inner` classes have), so it does not hurt to scope the enum to the command where it is being used. Signed-off-by: Sebastian Schuberth --- plugins/commands/compare/src/main/kotlin/CompareCommand.kt | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/plugins/commands/compare/src/main/kotlin/CompareCommand.kt b/plugins/commands/compare/src/main/kotlin/CompareCommand.kt index 62701887b8c55..7a982a64b6ad2 100644 --- a/plugins/commands/compare/src/main/kotlin/CompareCommand.kt +++ b/plugins/commands/compare/src/main/kotlin/CompareCommand.kt @@ -51,6 +51,8 @@ class CompareCommand : OrtCommand( name = "compare", help = "Compare two ORT results with various methods." ) { + private enum class CompareMethod { SEMANTIC_DIFF, TEXT_DIFF } + private val fileA by argument(help = "The first ORT result file to compare.") .convert { it.expandTilde() } .file(mustExist = true, canBeFile = true, canBeDir = false, mustBeWritable = false, mustBeReadable = true) @@ -166,11 +168,6 @@ class CompareCommand : OrtCommand( } } -private enum class CompareMethod { - SEMANTIC_DIFF, - TEXT_DIFF -} - private class EpochInstantDeserializer : StdDeserializer(Instant::class.java) { override fun deserialize(parser: JsonParser, context: DeserializationContext): Instant = Instant.EPOCH.also { From a816364a741c81ad81bacde4042c511bab558d82 Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Tue, 28 Nov 2023 14:36:14 +0100 Subject: [PATCH 3/5] refactor(compare-command): Rename `deserializer` to `mapper` Use a more general name as this is now used for serializing, too. Signed-off-by: Sebastian Schuberth --- .../commands/compare/src/main/kotlin/CompareCommand.kt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/plugins/commands/compare/src/main/kotlin/CompareCommand.kt b/plugins/commands/compare/src/main/kotlin/CompareCommand.kt index 7a982a64b6ad2..42065dac14c60 100644 --- a/plugins/commands/compare/src/main/kotlin/CompareCommand.kt +++ b/plugins/commands/compare/src/main/kotlin/CompareCommand.kt @@ -97,7 +97,7 @@ class CompareCommand : OrtCommand( } // Arbitrarily determine the mapper from the first file as the file extensions are ensured to be the same. - val deserializer = fileA.mapper().registerModule( + val mapper = fileA.mapper().registerModule( SimpleModule().apply { // TODO: Find a way to also ignore temporary directories (when diffing semantically). if (ignoreTime) addDeserializer(Instant::class.java, EpochInstantDeserializer()) @@ -105,8 +105,8 @@ class CompareCommand : OrtCommand( } ) - val resultA = deserializer.readValue(fileA) - val resultB = deserializer.readValue(fileB) + val resultA = mapper.readValue(fileA) + val resultB = mapper.readValue(fileB) when (method) { CompareMethod.SEMANTIC_DIFF -> { @@ -126,8 +126,8 @@ class CompareCommand : OrtCommand( } CompareMethod.TEXT_DIFF -> { - val textA = deserializer.writeValueAsString(resultA) - val textB = deserializer.writeValueAsString(resultB) + val textA = mapper.writeValueAsString(resultA) + val textB = mapper.writeValueAsString(resultB) // Apply data type independent replacements in the texts. val replacements = buildMap { From e85b5ebce4adc3ecca1843a6d722386482cc301d Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Tue, 28 Nov 2023 14:42:09 +0100 Subject: [PATCH 4/5] fix(compare-command): Fix the program exit codes Use code 1 for fundamental errors that prevent the command from properly executing, and codes >= 2 to signal failures detected as part of running the command properly. Signed-off-by: Sebastian Schuberth --- plugins/commands/compare/src/main/kotlin/CompareCommand.kt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/commands/compare/src/main/kotlin/CompareCommand.kt b/plugins/commands/compare/src/main/kotlin/CompareCommand.kt index 42065dac14c60..f1559ccd04329 100644 --- a/plugins/commands/compare/src/main/kotlin/CompareCommand.kt +++ b/plugins/commands/compare/src/main/kotlin/CompareCommand.kt @@ -93,7 +93,7 @@ class CompareCommand : OrtCommand( if (fileA.extension != fileB.extension) { echo("The file arguments need to be of the same type.") - throw ProgramResult(2) + throw ProgramResult(1) } // Arbitrarily determine the mapper from the first file as the file extensions are ensured to be the same. @@ -122,7 +122,7 @@ class CompareCommand : OrtCommand( throw ProgramResult(0) } - throw ProgramResult(1) + throw ProgramResult(2) } CompareMethod.TEXT_DIFF -> { @@ -162,7 +162,7 @@ class CompareCommand : OrtCommand( echo(it) } - throw ProgramResult(1) + throw ProgramResult(2) } } } From b0680671b7e2212164bea29683f2335386fa1e92 Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Tue, 28 Nov 2023 14:46:11 +0100 Subject: [PATCH 5/5] feat(compare-command): Make the context size configurable via an option Signed-off-by: Sebastian Schuberth --- .../commands/compare/src/main/kotlin/CompareCommand.kt | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/plugins/commands/compare/src/main/kotlin/CompareCommand.kt b/plugins/commands/compare/src/main/kotlin/CompareCommand.kt index f1559ccd04329..c39311424aa46 100644 --- a/plugins/commands/compare/src/main/kotlin/CompareCommand.kt +++ b/plugins/commands/compare/src/main/kotlin/CompareCommand.kt @@ -34,6 +34,7 @@ import com.github.ajalt.clikt.parameters.options.flag import com.github.ajalt.clikt.parameters.options.option import com.github.ajalt.clikt.parameters.types.enum import com.github.ajalt.clikt.parameters.types.file +import com.github.ajalt.clikt.parameters.types.int import com.github.ajalt.mordant.rendering.Theme import com.github.difflib.DiffUtils import com.github.difflib.UnifiedDiffUtils @@ -70,6 +71,13 @@ class CompareCommand : OrtCommand( ).enum() .default(CompareMethod.TEXT_DIFF) + private val contextSize by option( + "--context-size", "-C", + help = "The number of unmodified lines to display in the context of a modified line. Only applies to unified " + + "diff output." + ).int() + .default(7) + private val ignoreTime by option( "--ignore-time", "-t", help = "Ignore time differences." @@ -148,7 +156,7 @@ class CompareCommand : OrtCommand( "b/${fileB.relativeTo(commonParent).invariantSeparatorsPath}", linesA, DiffUtils.diff(linesA, linesB), - /* contextSize = */ 7 + contextSize ) if (diff.isEmpty()) {