[MNG-8015] Adjustments in new API related to PathType #1501
Conversation
…y more specific standard methods.
* Add a `warningForFilenameBasedAutomodules()` method in `DependencyResolverResult`. * Add relationships from `JavaPathType` to `javax.tool` location API. * Modify the `PathType.option(Iterable<? extends Path>)` return type because the option and the value need to be two separated arguments.
| @@ -18,6 +18,10 @@ | |||
| */ | |||
| package org.apache.maven.api; | |||
|
|
|||
| import javax.tools.DocumentationTool; | |||
There was a problem hiding this comment.
api shouldn't depend on that since it is a generic path setup and there we make it depending on a single consumer so look fishy and not very promishing to be as generic as intended
There was a problem hiding this comment.
The generic API is the PathType interface. This dependency appears in th JavaPathType enumeration, which is the specialization for the Java tools.
There was a problem hiding this comment.
Ok for me if the name reflects it so something like JavaxToolPathType - or whatever makes it explicit. JavaPathType is way more generic IMHO.
There was a problem hiding this comment.
"Java" here refers to standard Java as defined by Oracle (other entities must use other names, e.g. "Jakarta"). The use of standard javax.tools API should not block non-standard Java tools to be added to this JavaPathType enumeration if desired, because Location is an interface with 2 non-default methods easy to implement (getName() and isOutputLocation()), and the proposed JavaPathType.location() property is Optional anyway.
The close connection to javax.tools API may actually be desirable because contrarily to other API, javax.tools is updated immediately when new Java features requires compiler changes. For example, javax.tools has been updated in Java 9 with the addition of new methods specific to modules management. By contrast, Maven and the Plexus compiler did not really reflected that evolution as far as I can see.
api/maven-api-core/src/main/java/org/apache/maven/api/JavaPathType.java
Outdated
Show resolved
Hide resolved
api/maven-api-core/src/main/java/org/apache/maven/api/JavaPathType.java
Outdated
Show resolved
Hide resolved
| * @throws IllegalStateException if no option is associated to this path type | ||
| */ | ||
| @Nonnull | ||
| @Override | ||
| public String option(Iterable<? extends Path> paths) { | ||
| public String[] option(Iterable<? extends Path> paths) { |
There was a problem hiding this comment.
I wonder if this method should be moved out of the API.
There was a problem hiding this comment.
The presence or not of this method depends on the comment below.
| */ | ||
| @Nonnull | ||
| String option(Iterable<? extends Path> paths); | ||
| String[] option(Iterable<? extends Path> paths); |
There was a problem hiding this comment.
I wonder if this method should be moved out of the API.
There was a problem hiding this comment.
The method is proposed here for two reasons: code reuse, and because the method behaviour depends on the option rather than the plugin using it.
Code reuse
The use of this method would be shared by many plugins. For example, the same --class-path option is used by java, javac, javadoc, javap, jdeprscan, jdeps, jmod and jshell. In current Maven architecture, those tools are handled by independent plugins. This method reduces the need to repeat the same code in all plugins.
Note: this strategy relies on Oracle using consistently the same option names for all tools. It seems to be the case for all tools except serialver, provided that we use the modern form (e.g. --class-path instead of -cp).
Who defines the behaviour
The behaviour of this method depends on the option rather than the plugin. For example, JavaPathType.CLASSES formats the given list of files as --class-path <the files>. But JavaPathType.patchModule("org.my.module") formats the same list of files as --patch-module org.my.module=<the files>. This rule is applied consistently in at least java, javac and javadoc. As we can see, the actual work done by this method is determined by the option, not by the plugin, which is why the method appears in PathType: for allowing the implementation to be option-dependent.
api/maven-api-core/src/main/java/org/apache/maven/api/Type.java
Outdated
Show resolved
Hide resolved
api/maven-api-core/src/main/java/org/apache/maven/api/Type.java
Outdated
Show resolved
Hide resolved
api/maven-api-core/src/main/java/org/apache/maven/api/Type.java
Outdated
Show resolved
Hide resolved
gnodet
changed the title
This pull request makes the following changes in API related to
PathType. Those changes are for supporting the development of the Maven compiler plugin:PROCESSORMODULAR_PROCESSORCLASSPATH_PROCESSORorg.apache.maven.api.JavaPathTypeandjavax.tools.StandardLocation:location()public method returningOptional<JavaFileManager.Location>.valueOf(JavaFileManager.Location location)public static method (converse of the above).option(Iterable<? extends Path> paths)fromStringtoString[].DependencyResolverResult.option(PathType)because it appeared to not be needed.DependencyResolverResult.warningForFilenameBasedAutomodules()as a helper for managing the "Filename-based automodules detected on the module-path: Please don't publish this project to a public artifact repository" warning. This warning already exists in Maven 3.All methods removed or changed were new in Maven 4-alpha, so there is no compatibility break with stable releases.