gnodet-bot commented on code in PR #13137: URL: https://github.com/apache/maven/pull/13137#discussion_r4014788904
########## impl/maven-core/src/main/java/org/apache/maven/lifecycle/PluginVersions.java: ########## @@ -0,0 +1,81 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.lifecycle; + +import java.io.IOException; +import java.io.InputStream; +import java.util.Properties; + +/** + * Provides default plugin versions for the built-in lifecycle bindings. + * <p> + * Versions are read from {@code plugin-versions.properties}, which is filtered + * at build time from POM properties ({@code version.maven-<name>-plugin}). + * Centralising them in the POM makes them visible to dependency-update bots + * such as Dependabot and Renovate. + * + * @since 4.1.0 Review Comment: ⚠️ **Wrong `@since` version for a backport to `maven-4.0.x`.** This class is introduced on `maven-4.0.x` (project version `4.0.0-SNAPSHOT`). The `@since 4.1.0` tag was correct in the original #13080 targeting `master`, but here it should reflect the version where this class first appears on this branch. ```suggestion * @since 4.0.0 ``` ########## impl/maven-core/src/main/java/org/apache/maven/lifecycle/providers/packaging/AbstractLifecycleMappingProvider.java: ########## @@ -35,29 +36,51 @@ */ public abstract class AbstractLifecycleMappingProvider implements Provider<LifecycleMapping> { // START SNIPPET: versions - protected static final String RESOURCES_PLUGIN_VERSION = "3.3.1"; + /** @deprecated Use {@link PluginVersions#RESOURCES} instead. */ + @Deprecated(since = "4.1.0", forRemoval = true) Review Comment: ⚠️ **Wrong `@Deprecated(since = ...)` version — applies to all 11 annotations.** All 11 `@Deprecated(since = "4.1.0", forRemoval = true)` annotations have the wrong version. The deprecation is being introduced in this PR, which targets `maven-4.0.x`. The correct value is `"4.0.0"`. This matters: `since` documents the first version in which the element became deprecated. Callers inspecting the annotation at runtime or via tooling will see a version that doesn't exist on this branch. ```suggestion @Deprecated(since = "4.0.0", forRemoval = true) ``` (Apply the same fix to all 11 occurrences on lines 40, 44, 48, 52, 56, 62, 66, 70, 74, 78, 82.) ########## impl/maven-core/src/main/java/org/apache/maven/lifecycle/providers/packaging/AbstractLifecycleMappingProvider.java: ########## @@ -35,29 +36,51 @@ */ public abstract class AbstractLifecycleMappingProvider implements Provider<LifecycleMapping> { // START SNIPPET: versions - protected static final String RESOURCES_PLUGIN_VERSION = "3.3.1"; + /** @deprecated Use {@link PluginVersions#RESOURCES} instead. */ + @Deprecated(since = "4.1.0", forRemoval = true) Review Comment: ⚠️ **Subclasses not migrated — deprecated constants immediately used by their own callers.** All 8 subclasses in this package (`BomLifecycleMappingProvider`, `EarLifecycleMappingProvider`, `EjbLifecycleMappingProvider`, `JarLifecycleMappingProvider`, `MavenPluginLifecycleMappingProvider`, `PomLifecycleMappingProvider`, `RarLifecycleMappingProvider`, `WarLifecycleMappingProvider`) still reference the deprecated inherited constants (e.g. `RESOURCES_PLUGIN_VERSION`, `COMPILER_PLUGIN_VERSION`) directly. The net result is that the module deprecates constants as `forRemoval = true` and then immediately continues using them — the deprecation is self-contradictory. Either: 1. **Migrate all subclasses to `PluginVersions.*` in this PR** (preferred — they are all in the same package, the change is mechanical, and it completes the intent of the refactoring), or 2. **Don't add `forRemoval = true`** until a follow-up PR migrates the consumers. Option 1 is the right call — the constants exist solely to be used by these 8 subclasses. -- 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]
