gnodet-bot commented on code in PR #13613: URL: https://github.com/apache/maven/pull/13613#discussion_r4326461564
########## api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelDialectManager.java: ########## @@ -0,0 +1,77 @@ +/* + * 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.api.services; + +import java.nio.file.Path; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +import org.apache.maven.api.Dialect; +import org.apache.maven.api.Service; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.annotations.Nullable; +import org.apache.maven.api.model.Model; + +/** + * The {@code ModelDialectManager} provides a mechanism for managing and + * translating between different dialects of Maven models. + * It allows reading and writing models in various dialects supported by the system. + * + * @since 4.1.0 + */ +public interface ModelDialectManager extends Service { + + /** + * Retrieves the set of all available dialects. + * The result is never {@code null} and always contains at least one element, + * the core {@link Dialect#XML} dialect. + * + * @return a non-null {@link Set} of available dialects. + */ + @Nonnull + Set<Dialect> getAvailableDialects(); + + /** + * Reads a Maven model from the specified directory using the specified dialect. + * + * @param dir the directory from which to read the model, must not be {@code null}. + * @param dialect the dialect to be used for reading, must not be {@code null}. + * @param options optional parameters for reading, may be {@code null}. + * @return an {@link Optional} containing the read model, or an empty {@link Optional} if reading fails. Review Comment: 📝 **Unaddressed (previous review):** The `@return` says _"or an empty `Optional` if reading fails"_, but that's inaccurate. The implementation delegates to `ModelParser.locateAndParse(dir, options)` which returns empty only when `locate()` finds **no file** in the directory — not when reading fails. Actual parse failures throw `ModelParserException` (unchecked). The Javadoc conflates "file not found" with "read error", which will mislead callers who catch the wrong thing. ```suggestion * @return an {@link Optional} containing the read model, or an empty {@link Optional} if no model * file was found in the directory for the given dialect. * @throws org.apache.maven.api.spi.ModelParserException if the model file is found but cannot be parsed. * @throws IllegalArgumentException if an unrecognized dialect is provided. ``` ########## api/maven-api-spi/src/main/java/org/apache/maven/api/spi/ModelDialectProvider.java: ########## @@ -0,0 +1,66 @@ +/* + * 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.api.spi; + +import org.apache.maven.api.Dialect; +import org.apache.maven.api.annotations.Consumer; +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.di.Named; + +/** + * SPI to provide a symmetric pair of {@link ModelParser} and {@link ModelWriter} for a given {@link Dialect}. + * <p> + * Implement this interface to register support for reading and writing Maven models in an alternative dialect. + * The implementation must be registered as a named DI component so that Maven can discover it. + * + * @since 4.1.0 + * @see ModelParser + * @see ModelWriter + * @see DialectProvider + */ +@Experimental +@Consumer +@Named +public interface ModelDialectProvider { Review Comment: 💡 **Unaddressed (previous review):** `ModelParser` extends `SpiService` and `ModelWriter` extends `SpiService`, but `ModelDialectProvider` — which composes both — does not. An extension author looking at the SPI discovery contract gets an inconsistent signal: the two leaf SPIs opt into `SpiService`, the composing SPI does not. ```suggestion public interface ModelDialectProvider extends SpiService { ``` ########## api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelDialectManager.java: ########## @@ -0,0 +1,77 @@ +/* + * 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.api.services; + +import java.nio.file.Path; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +import org.apache.maven.api.Dialect; +import org.apache.maven.api.Service; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.annotations.Nullable; +import org.apache.maven.api.model.Model; + +/** + * The {@code ModelDialectManager} provides a mechanism for managing and + * translating between different dialects of Maven models. + * It allows reading and writing models in various dialects supported by the system. + * + * @since 4.1.0 + */ +public interface ModelDialectManager extends Service { + + /** + * Retrieves the set of all available dialects. + * The result is never {@code null} and always contains at least one element, + * the core {@link Dialect#XML} dialect. + * + * @return a non-null {@link Set} of available dialects. + */ + @Nonnull + Set<Dialect> getAvailableDialects(); + + /** + * Reads a Maven model from the specified directory using the specified dialect. + * + * @param dir the directory from which to read the model, must not be {@code null}. + * @param dialect the dialect to be used for reading, must not be {@code null}. + * @param options optional parameters for reading, may be {@code null}. + * @return an {@link Optional} containing the read model, or an empty {@link Optional} if reading fails. + * @throws IllegalArgumentException if an unrecognized dialect is provided. + */ + @Nonnull + Optional<Model> readModel(@Nonnull Path dir, @Nonnull Dialect dialect, @Nullable Map<String, ?> options); Review Comment: 📝 **Missing `@throws` documentation:** `readModel` can throw `ModelParserException` (unchecked, via `locateAndParse`) and `writeModel` can throw `ModelWriterException` (unchecked, via `targetAndWrite`). Neither is documented in the interface Javadoc. Since `ModelDialectManager` is the API contract, callers need to know what exceptions to expect beyond `IllegalArgumentException`. The `@return` fix above for `readModel` should include the `@throws ModelParserException` clause; `writeModel` needs a matching `@throws ModelWriterException`. ```suggestion * @throws org.apache.maven.api.spi.ModelParserException if the model file is found but cannot be parsed. * @throws IllegalArgumentException if an unrecognized dialect is provided. */ @Nonnull Optional<Model> readModel(@Nonnull Path dir, @Nonnull Dialect dialect, @Nullable Map<String, ?> options); ``` ########## api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelDialectManager.java: ########## @@ -0,0 +1,77 @@ +/* + * 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.api.services; + +import java.nio.file.Path; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +import org.apache.maven.api.Dialect; +import org.apache.maven.api.Service; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.annotations.Nullable; +import org.apache.maven.api.model.Model; + +/** + * The {@code ModelDialectManager} provides a mechanism for managing and + * translating between different dialects of Maven models. + * It allows reading and writing models in various dialects supported by the system. + * + * @since 4.1.0 + */ +public interface ModelDialectManager extends Service { Review Comment: ⚠️ **Unaddressed (previous review):** `ModelDialectManager` is the only new `Service` interface in this PR without `@Experimental`. All other new types carry it — `Dialect`, `DialectProvider`, `ModelWriter`, `ModelDialectProvider`, `ModelWriterException` — as do existing services like `ArtifactManager` and `DependencyResolver`. Fix: ```suggestion @org.apache.maven.api.annotations.Experimental public interface ModelDialectManager extends Service { ``` -- 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]
