gnodet commented on PR #13080:
URL: https://github.com/apache/maven/pull/13080#issuecomment-5613684983
Thanks @ascheman for the thorough review, and @elharo for the inline
suggestion. Let me address each point:
**1. Triplication**
I hear the concern, but I think it's somewhat overstated. The three "copies"
aren't really independent sources of truth — they're a pipeline: `POM property
→ filtered .properties file → Java constant`. Each layer serves a distinct
purpose:
- POM property: bot-visible, build-time source of truth
- .properties file: carries the value into the classpath (bridge between
Maven build and Java runtime)
- Java constant: type-safe, IDE-friendly, refactor-safe access for callers
Dropping the constants in favor of raw `version("maven-clean-plugin")` calls
would trade compile-time safety for stringly-typed lookups — a typo in the
artifact ID silently compiles and only blows up at runtime. The constants catch
that at the first build.
That said, I agree the error-prone scenario (adding a plugin to the POM but
forgetting the .properties or the constant) should be guarded. The unit test
suggested in the earlier review (and your point 3) covers exactly that.
**2. Dependabot / Renovate effectiveness**
This is the strongest point, and you're right to flag it. Free-standing POM
properties not referenced by any `<dependency>` or `<plugin>` **will not** be
picked up by Dependabot's Maven ecosystem — it only tracks properties used in
declared dependency/plugin version elements. Renovate has the same limitation.
Two options to fix this:
- (a) Add a `<pluginManagement>` block in the same POM that declares all 13
plugins with `${version.maven-*-plugin}` — the bots would then see real plugin
declarations referencing the properties and propose bumps. This is the simplest
path.
- (b) Use Renovate's `regexManagers` or Dependabot's `custom` ecosystem with
regex — fragile and repo-specific.
I'll go with (a) in the next push.
**3. Test**
Agreed — this was already raised in the initial review. I'll add a unit test
that reflectively checks all `public static final String` fields are non-null
and not unfiltered placeholders.
**Nit (key naming)**
Good catch. I'll align: `version.maven-clean-plugin` in the POM →
`version.maven-clean-plugin` in the properties file too (dropping the
`.version` suffix / using the same key).
**@elharo's suggestion (deprecate/remove fields)**
Since `AbstractLifecycleMappingProvider` is `protected` API (subclassed by
packaging providers, potentially by extensions), the safe path for the `master`
branch (4.1.0) is to deprecate the fields and delegate to `PluginVersions.*`.
I'll add `@Deprecated(since = "4.1.0", forRemoval = true)` in the next push.
---
Next push will address: placeholder guard, `private version()`, unit test,
`<pluginManagement>` for bot visibility, key naming alignment, and field
deprecation.
_This comment was generated by an AI agent, Hermès, on behalf of @gnodet._
--
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]
