[MNG-7836] Support alternative syntaxes for POMs #1197

gnodet · 2023-07-05T06:33:22Z

JIRA issue: https://issues.apache.org/jira/browse/MNG-7836
IT PR: apache/maven-integration-testing#276

api/maven-api-core/src/main/java/org/apache/maven/api/services/Source.java

api/maven-api-spi/src/main/java/org/apache/maven/api/spi/ModelParser.java

...e/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java

maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelProcessor.java

maven-model-builder/src/main/java/org/apache/maven/model/building/FileSource.java

api/maven-api-spi/src/main/java/org/apache/maven/api/spi/ModelParser.java

api/maven-api-core/src/main/java/org/apache/maven/api/services/Source.java

api/maven-api-spi/src/main/java/org/apache/maven/api/spi/ModelParser.java

The IT associated with this PR is using the Maven model to generate a hocon POM parser. This requires the maven-api-model module to attach the POM as an artifact, and the maven.yml change so that the model is present in the local repository.

elharo

I'll move this to a comment instead of a request changes, but I still think there are pretty serious issues between checked and runtime exceptions here, and those need to be resolved ASAP, and certainly before the 4.0 API is locked down and ships.

elharo · 2023-09-19T11:19:27Z

api/maven-api-spi/src/main/java/org/apache/maven/api/spi/ModelParserException.java

+import org.apache.maven.api.services.MavenException;
+
+@Experimental
+public class ModelParserException extends MavenException {


I'm tempted to say that either this shouldn't extend MavenException then, or that this PR should wait ion resolving that issue. It's a pretty serious API design problem. This is a new exception in 4.0, right?

elharo · 2023-09-19T11:20:51Z

maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelProcessor.java

+                .filter(Objects::nonNull)
+                .findFirst()
+                .orElseGet(
+                        () -> modelLocator.locatePom(projectDirectory.toFile()).toPath());
        if (!pom.equals(projectDirectory) && !pom.getParent().equals(projectDirectory)) {
            throw new IllegalArgumentException("The POM found does not belong to the given directory: " + pom);


I'm not sure this is the right exception here. This isn't necessarily a problem with he argument itself. It's more akin to a checked IOException, again arising from conditions external to the program itself.

I strongly disagree. This is a health check on the response. Both ModelLocator#locatePom and ModelParser#locate are supposed to return a file in the provided directory or the directory itself. This is quite explicit in their respective javadoc. So it's a programming error and can not be recovered.

elharo · 2023-09-19T11:21:31Z

maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelProcessor.java

+                .filter(Objects::nonNull)
+                .findFirst()
+                .orElseGet(() -> {
+                    File f = modelLocator.locateExistingPom(projectDirectory.toFile());


probably don't need to convert to a File here, just use the Path

ModelLocator is part of the Maven 3 public api and uses the File instead of Path.
The Maven 4 api uses Path and not File, but I don't think that's a good idea to start modifying the Maven 3 api in incompatible ways, or even to duplicate all the methods to leverage the nio API at this point.
So I'm not really sure what can be done here.

Fair enough, on File then. That makes sense.

On the exception, whether it can be recovered is not relevant. That is not the determining factor in whether a checked exception is appropriate.

@elharo I agree with @gnodet that checked exceptions should be used if calling code can do something about it (not necessarily "recover") and should do something, programming errors are usually nothing code can act uppon and will only result in try-catch-forget ...

I know this is a common belief, but it's not how exceptions in Java were designed and it doesn't work in practice.

https://www.youtube.com/watch?v=rJ-Ihh7RNao

unblocking

gnodet · 2023-09-19T13:49:36Z

I'll move this to a comment instead of a request changes, but I still think there are pretty serious issues between checked and runtime exceptions here, and those need to be resolved ASAP, and certainly before the 4.0 API is locked down and ships.

I don't think there are issues at this point, as this was a concious design decision. A change can be discussed, feel free to start a discussion and make your point.

gnodet changed the title ~~MNG 7836 Support alternative syntaxes for POMs~~ [MNG 7836] Support alternative syntaxes for POMs Jul 5, 2023

gnodet force-pushed the MNG-7836-Support-alternative-syntaxes-for-POMs branch from c873f5b to 3385569 Compare July 5, 2023 09:03

elharo previously requested changes Jul 5, 2023

View reviewed changes

gnodet changed the title ~~[MNG 7836] Support alternative syntaxes for POMs~~ [MNG-7836] Support alternative syntaxes for POMs Jul 11, 2023

gnodet mentioned this pull request Jul 11, 2023

[MNG-7836] IT for alternative pom syntax apache/maven-integration-testing#276

Merged

gnodet force-pushed the MNG-7836-Support-alternative-syntaxes-for-POMs branch from eff9877 to 709e9d6 Compare July 12, 2023 12:24

gnodet requested a review from cstamas July 20, 2023 06:04

gnodet force-pushed the MNG-7836-Support-alternative-syntaxes-for-POMs branch from 709e9d6 to 2e669bb Compare July 20, 2023 07:06

gnodet force-pushed the MNG-7836-Support-alternative-syntaxes-for-POMs branch 2 times, most recently from e423eb1 to b757bb5 Compare August 21, 2023 14:27

laeubi reviewed Aug 23, 2023

View reviewed changes

api/maven-api-spi/src/main/java/org/apache/maven/api/spi/ModelParser.java Outdated Show resolved Hide resolved

gnodet force-pushed the MNG-7836-Support-alternative-syntaxes-for-POMs branch from b757bb5 to 97c11dd Compare August 24, 2023 11:44

laeubi reviewed Aug 29, 2023

View reviewed changes

gnodet force-pushed the MNG-7836-Support-alternative-syntaxes-for-POMs branch 3 times, most recently from dca4063 to c7662cb Compare September 11, 2023 18:44

gnodet force-pushed the MNG-7836-Support-alternative-syntaxes-for-POMs branch from c7662cb to 2890d61 Compare September 13, 2023 11:49

gnodet added this to the 4.0.0-alpha-8 milestone Sep 14, 2023

gnodet requested review from elharo and slawekjaranowski September 19, 2023 07:25

elharo reviewed Sep 19, 2023

View reviewed changes

cstamas approved these changes Sep 21, 2023

View reviewed changes

gnodet merged commit f24266e into apache:master Sep 22, 2023
18 checks passed

gnodet deleted the MNG-7836-Support-alternative-syntaxes-for-POMs branch September 22, 2023 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MNG-7836] Support alternative syntaxes for POMs #1197

[MNG-7836] Support alternative syntaxes for POMs #1197

gnodet commented Jul 5, 2023 •

edited

Loading

elharo left a comment

elharo Sep 19, 2023

elharo Sep 19, 2023

gnodet Sep 19, 2023

elharo Sep 19, 2023

gnodet Sep 19, 2023

elharo Sep 19, 2023

laeubi Sep 20, 2023

elharo Sep 20, 2023

gnodet commented Sep 19, 2023

[MNG-7836] Support alternative syntaxes for POMs #1197

[MNG-7836] Support alternative syntaxes for POMs #1197

Conversation

gnodet commented Jul 5, 2023 • edited Loading

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnodet commented Sep 19, 2023

gnodet commented Jul 5, 2023 •

edited

Loading