[GitHub] [maven-doxia-sitetools] michael-o commented on a diff in pull request #94: [DOXIASITETOOLS-294] Replace legacy artifact resolution with Maven Resolver

["michael-o (via GitHub)" <gi...@apache.org>]

michael-o commented on code in PR #94:
URL: https://github.com/apache/maven-doxia-sitetools/pull/94#discussion_r1141029571


##########
doxia-integration-tools/src/main/java/org/apache/maven/doxia/tools/DefaultSiteTool.java:
##########
@@ -806,129 +790,98 @@ protected static String getNormalizedPath(String path) {
      */
     private File resolveSiteDescriptor(
             MavenProject project,
-            ArtifactRepository localRepository,
-            List<ArtifactRepository> repositories,
+            RepositorySystemSession repoSession,
+            List<ArtifactRepository> remoteArtifactRepositories,
             Locale locale)
             throws IOException, ArtifactResolutionException, ArtifactNotFoundException {
         String variant = locale.getVariant();
         String country = locale.getCountry();
         String language = locale.getLanguage();
 
-        Artifact artifact = null;
+        String localeStr = null;
         File siteDescriptor = null;
         boolean found = false;
 
         if (!variant.isEmpty()) {
-            String localeStr = language + "_" + country + "_" + variant;
-            // TODO: this is a bit crude - proper type, or proper handling as metadata rather than an artifact in 2.1?
-            artifact = artifactFactory.createArtifactWithClassifier(
-                    project.getGroupId(), project.getArtifactId(), project.getVersion(), "xml", "site_" + localeStr);
+            localeStr = language + "_" + country + "_" + variant;
+            ArtifactRequest request =
+                    createSiteDescriptorArtifactRequest(project, localeStr, remoteArtifactRepositories);
 
             try {
-                artifactResolver.resolve(artifact, repositories, localRepository);
-
-                siteDescriptor = artifact.getFile();
+                ArtifactResult result = repositorySystem.resolveArtifact(repoSession, request);
 
-                // we use zero length files to avoid re-resolution (see below)
-                if (siteDescriptor.length() > 0) {
-                    found = true;
-                } else {
+                // TODO Can "result.isMissing()" happen here?
+                siteDescriptor = result.getArtifact().getFile();
+                found = true;
+            } catch (ArtifactResolutionException e) {
+                if (e.getCause() instanceof ArtifactNotFoundException) {
                     LOGGER.debug("No site descriptor found for '" + project.getId() + "' for locale '" + localeStr
                             + "', trying without variant...");
+                } else {
+                    throw e;
                 }
-            } catch (ArtifactNotFoundException e) {
-                LOGGER.debug("Unable to locate site descriptor for locale '" + localeStr + "'", e);
-
-                // we can afford to write an empty descriptor here as we don't expect it to turn up later in the
-                // remote repository, because the parent was already released (and snapshots are updated
-                // automatically if changed)
-                siteDescriptor = new File(localRepository.getBasedir(), localRepository.pathOf(artifact));
-                siteDescriptor.getParentFile().mkdirs();
-                siteDescriptor.createNewFile();
             }
         }
 
         if (!found && !country.isEmpty()) {
-            String localeStr = language + "_" + country;
-            // TODO: this is a bit crude - proper type, or proper handling as metadata rather than an artifact in 2.1?
-            artifact = artifactFactory.createArtifactWithClassifier(
-                    project.getGroupId(), project.getArtifactId(), project.getVersion(), "xml", "site_" + localeStr);
+            localeStr = language + "_" + country;
+            ArtifactRequest request =
+                    createSiteDescriptorArtifactRequest(project, localeStr, remoteArtifactRepositories);
 
             try {
-                artifactResolver.resolve(artifact, repositories, localRepository);
+                ArtifactResult result = repositorySystem.resolveArtifact(repoSession, request);
 
-                siteDescriptor = artifact.getFile();
-
-                // we use zero length files to avoid re-resolution (see below)
-                if (siteDescriptor.length() > 0) {
-                    found = true;
-                } else {
+                // TODO Can "result.isMissing()" happen here?

Review Comment:
   I tried the offline case, no file means failure with exception. I will remove the TODOs until proven otherwise.



-- 
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: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org