gnodet-bot commented on code in PR #1123:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1123#discussion_r4084544948
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -586,7 +586,16 @@ final Charset charset() {
protected String outputTimestamp;
/**
- * The algorithm to use for selecting which files to compile.
+ * <b>Despite the word "incremental" in the name, this is <i>not</i> an
incremental compiler
+ * in the sense of an IDE.</b> The plugin does not compile a single
changed class and the classes
+ * that depend on it. It selects an algorithm used to <i>detect
changes</i> and to decide whether
+ * to recompile the whole module or only some source files. In most
configurations a change causes
+ * the whole module (all its source files) to be recompiled; see the
{@code sources} and
+ * {@code classes} values below, which decide whether only the modified
source files are recompiled
+ * or whether a change triggers a full rebuild. The plugin never performs
dependency-based
+ * compilation of only the directly or transitively affected classes.
Review Comment:
⚠️ **"Never" is too absolute given the `modules` algorithm**
The claim "The plugin never performs dependency-based compilation of only
the directly or transitively affected classes" is not accurate when
`incrementalCompilation` includes `modules`. The `modules` algorithm passes
`--module` to `javac` and explicitly defers the per-file recompilation decision
to the compiler itself — which can and does perform dependency-aware
recompilation within the module. The existing Javadoc for the `modules` value
already says "let the compiler decides which individual files to recompile."
The "never" should be scoped to the non-`modules` algorithms, or the
sentence should be removed in favour of the already-present
`<h4>Limitations</h4>` section which phrases this more accurately as "does not
detect structural changes other than file addition or removal."
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -586,7 +586,16 @@ final Charset charset() {
protected String outputTimestamp;
/**
- * The algorithm to use for selecting which files to compile.
+ * <b>Despite the word "incremental" in the name, this is <i>not</i> an
incremental compiler
+ * in the sense of an IDE.</b> The plugin does not compile a single
changed class and the classes
+ * that depend on it. It selects an algorithm used to <i>detect
changes</i> and to decide whether
+ * to recompile the whole module or only some source files. In most
configurations a change causes
+ * the whole module (all its source files) to be recompiled; see the
{@code sources} and
Review Comment:
⚠️ **Inaccuracy: "in most configurations a change causes the whole module to
be recompiled"**
This contradicts the actual default behavior. The default for
`incrementalCompilation` (without annotation processors on Java ≥ 23) is
`"options,dependencies,sources"`, which recompiles **only the modified source
files** unless the compiler options or a JAR dependency changed. A change to a
single `.java` file does **not** cause a full rebuild in the default
configuration.
The claim was more accurate for the v3 plugin (with
`useIncrementalCompilation=true` defaulting to
`dependencies,sources,rebuild-on-add`), but this Javadoc is on a `@since 4.0.0`
parameter. The current wording will mislead v4 users about how the default
configuration behaves.
Suggest replacing with something like: "Depending on the algorithm selected,
a change may cause only the modified source files to be recompiled, or may
trigger a full module rebuild."
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -670,7 +679,11 @@ final Charset charset() {
protected String incrementalCompilation;
/**
- * Whether to enable/disable incremental compilation feature.
+ * Whether to enable/disable the change detection that decides when to
recompile the module.
+ * Despite the word "incremental", this does not enable an
+ * incremental compiler in the sense of an IDE. The plugin never compiles
a single changed class
+ * together with the classes that depend on it. It only detects changes
and, depending on the
+ * configuration, recompiles the whole module or only the modified source
files.
Review Comment:
⚠️ **Accuracy concern raised in the existing thread — please address before
merging**
@desruisseaux's comment notes that in v4.x the original intent (IDE-style
incremental compilation) may actually be implemented now. If that's true,
documenting "this does not enable an incremental compiler in the sense of an
IDE" on `useIncrementalCompilation` (and by extension on
`incrementalCompilation`) would be incorrect on `master`.
Before merging, please confirm: does the `sources` algorithm in v4.x, or the
`modules` algorithm, actually perform class-level dependency tracking (i.e.
recompile `Bar.java` because `Foo.java` changed and `Bar` uses `Foo`)? If yes,
the Javadoc needs to be scoped to describe only the change-detection role, not
make an absolute claim about what IDE-style compilation means. The existing
`<h4>Limitations</h4>` section ("does not detect structural changes other than
file addition or removal") is already accurate and appropriately hedged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
