Conversation
Non-blocking FileReviewer that prints a colored unified diff to the test output, letting users inspect changes directly in IDE test panels or CI logs.
|
There was a problem hiding this comment.
Pull request overview
This PR adds a new review-console module that provides a non-blocking console-based file reviewer for ApproveJ. The ConsoleFileReviewer prints a colored unified diff to the test output, allowing developers to inspect approval differences directly in IDE test panels or CI logs without requiring an external diff tool.
Changes:
- Added
review-consolemodule with ConsoleFileReviewer implementation that uses java-diff-utils library - Implemented Terminal abstraction to support testability and NO_COLOR environment variable
- Added comprehensive unit tests covering colored/non-colored output and missing approved files
- Updated documentation to describe the new console review feature
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle.kts | Includes the new review-console module in the build |
| modules/review-console/build.gradle.kts | Defines build configuration for the new module with java-diff-utils dependency |
| gradle/libs.versions.toml | Adds java-diff-utils library dependency (version 4.15) with minor reordering |
| modules/review-console/src/main/java/org/approvej/review/console/ConsoleFileReviewer.java | Main reviewer implementation that generates and colorizes unified diffs |
| modules/review-console/src/main/java/org/approvej/review/console/Terminal.java | Abstraction for terminal output with color support detection |
| modules/review-console/src/main/java/org/approvej/review/console/NullableTerminal.java | In-memory terminal implementation for testing |
| modules/review-console/src/test/java/org/approvej/review/console/ConsoleFileReviewerTest.java | Unit tests covering diff output, coloring, and edge cases |
| modules/review-console/src/main/resources/META-INF/services/org.approvej.configuration.Provider | Service provider configuration for auto-discovery |
| manual/src/docs/asciidoc/chapters/06-reviewing.adoc | Documentation for the new console review feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final class NullableTerminal implements Terminal { | ||
|
|
||
| private final StringBuilder output = new StringBuilder(); | ||
| private final boolean supportsColor; | ||
|
|
||
| NullableTerminal() { | ||
| this(false); | ||
| } | ||
|
|
||
| NullableTerminal(boolean supportsColor) { |
There was a problem hiding this comment.
The class name "NullableTerminal" is misleading. This class is not about handling nullable values - it's an in-memory test double that captures output. Consider renaming it to "InMemoryTerminal" or "TestTerminal" to better reflect its purpose as a test fixture that stores output in memory.
| final class NullableTerminal implements Terminal { | |
| private final StringBuilder output = new StringBuilder(); | |
| private final boolean supportsColor; | |
| NullableTerminal() { | |
| this(false); | |
| } | |
| NullableTerminal(boolean supportsColor) { | |
| final class InMemoryTerminal implements Terminal { | |
| private final StringBuilder output = new StringBuilder(); | |
| private final boolean supportsColor; | |
| InMemoryTerminal() { | |
| this(false); | |
| } | |
| InMemoryTerminal(boolean supportsColor) { |
| assertj = "3.27.7" | ||
| jackson2 = "2.21.0" | ||
| jackson3 = "3.0.4" | ||
| junit = "6.0.3" | ||
| wiremock = "3.13.2" |
There was a problem hiding this comment.
The alphabetical ordering in the versions section has been disrupted. The "junit" entry was moved from line 1 to line 5, breaking the alphabetical order (assertj, jackson2, jackson3, junit). The entries should be sorted alphabetically: assertj, jackson2, jackson3, junit, wiremock.
| [source,java,indent=0] | ||
| ---- | ||
| .reviewedBy(ConsoleFileReviewer.console()) | ||
| ---- |
There was a problem hiding this comment.
The Console Review documentation section lacks consistency with other sections. The "Automatic Review" section (lines 48-57) includes both Java and Kotlin examples with role="primary" and role="secondary" tabs, while the Console Review section only shows a Java snippet without the tabbed structure. Consider adding a Kotlin example and using the same tabbed structure for consistency, or if tabs are not needed for this simpler example, explain why the pattern differs from other sections.
| [source,java,indent=0] | |
| ---- | |
| .reviewedBy(ConsoleFileReviewer.console()) | |
| ---- | |
| [source,java,indent=0,role="primary"] | |
| .Java | |
| ---- | |
| .reviewedBy(ConsoleFileReviewer.console()) | |
| ---- | |
| [source,kotlin,indent=0,role="secondary"] | |
| .Kotlin | |
| ---- | |
| .reviewedBy(ConsoleFileReviewer.console()) | |
| ---- |
|
|
||
| include("modules:http-wiremock") | ||
|
|
||
| include("modules:review-console") |
There was a problem hiding this comment.
The module ordering in settings.gradle.kts is not consistent. The modules appear to be grouped by type (json modules together, yaml modules together, http modules together), but "review-console" breaks this pattern by being placed after "http-wiremock" instead of after "core" (which would group review-related modules together) or in alphabetical order. Consider either: 1) placing it after "modules:core" to group review-related modules, or 2) maintaining strict alphabetical order within the modules section.
| @file:Suppress("UnstableApiUsage", "unused") | ||
|
|
||
| plugins { | ||
| `java-library` | ||
| jacoco | ||
| `jvm-test-suite` | ||
| `maven-publish` | ||
| } | ||
|
|
||
| java { | ||
| withJavadocJar() | ||
| withSourcesJar() | ||
| toolchain { languageVersion.set(JavaLanguageVersion.of(21)) } | ||
| } | ||
|
|
||
| repositories { mavenCentral() } | ||
|
|
||
| dependencies { | ||
| api(project(":modules:core")) | ||
| api(libs.jspecify) | ||
|
|
||
| implementation(libs.java.diff.utils) | ||
| } | ||
|
|
||
| testing { | ||
| suites { | ||
| val test by | ||
| getting(JvmTestSuite::class) { | ||
| useJUnitJupiter() | ||
| dependencies { | ||
| implementation(testFixtures(project(":modules:core"))) | ||
|
|
||
| implementation(platform(libs.junit.bom)) | ||
| implementation(libs.junit.jupiter.api) | ||
| implementation(libs.assertj.core) | ||
|
|
||
| runtimeOnly(libs.junit.platform.launcher) | ||
| runtimeOnly(libs.junit.jupiter.engine) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| tasks.jacocoTestReport { reports { xml.required = true } } |
There was a problem hiding this comment.
The review-console module is missing a gradle.properties file. All other modules in the codebase (e.g., modules/json-jackson/gradle.properties, modules/http/gradle.properties) include this file to define mavenPomName and mavenPomDescription. For consistency and proper Maven publication metadata, add a gradle.properties file with appropriate values like: mavenPomName = ApproveJ Console Review and mavenPomDescription = Console-based review support for ApproveJ.
Summary
review-consolemodule with a non-blockingConsoleFileReviewerthat prints a colored unified diff to the test outputconsolealias and respects theNO_COLORenvironment variableTest plan
./gradlew checkpasses.reviewedBy(ConsoleFileReviewer.console())in a failing approval test and inspect output