Respond to review comments

Author: shs96c

private/lib/coordinates.bzl

@@ -57,7 +57,7 @@ def unpack_coordinates(coords):
 def _is_version_number(part):
     return part[0].isdigit()
 
-def _unpack_if_necssary(coords):
+def _unpack_if_necessary(coords):
     if type(coords) == "string":
         unpacked = unpack_coordinates(coords)
     elif type(coords) == "dict":
@@ -80,7 +80,7 @@ def to_external_form(coords):
     syntax: `group:name:version:classifier@packaging`
     """
 
-    unpacked = _unpack_if_necssary(coords)
+    unpacked = _unpack_if_necessary(coords)
 
     to_return = "%s:%s:%s" % (unpacked.group, unpacked.artifact, unpacked.version)
 
@@ -97,7 +97,7 @@ def to_external_form(coords):
 # This matches the `Coordinates#asKey` method in the Java tree, and the
 # implementations must be kept in sync.
 def to_key(coords):
-    unpacked = _unpack_if_necssary(coords)
+    unpacked = _unpack_if_necessary(coords)
 
     key = unpacked.group + ":" + unpacked.artifact
 

private/rules/v2_lock_file.bzl

@@ -48,6 +48,8 @@ def _original_compute_lock_file_hash(lock_file_contents):
     return hash(repr(to_hash))
 
 def _compute_lock_file_hash(lock_file_contents):
+    # Note: this function is exactly equivalent to the one in `ArtifactsHash.java` if you
+    #       make a change there please make it here, and vice versa.
     lines = []
     artifacts = _get_artifacts(lock_file_contents)
 

private/tools/java/com/github/bazelbuild/rules_jvm_external/Coordinates.java

@@ -27,6 +27,26 @@ public class Coordinates implements Comparable<Coordinates> {
   private final String classifier;
   private final String extension;
 
+  /**
+   * Converts a `String` in one of two formats and extracts the information from it.
+   *
+   * <p>The two supported formats are:
+   *
+   * <ol>
+   *   <li>group:artifact:version:classifier@extension
+   *   <li>group:artifact:extension:classifier:version.
+   * </ol>
+   *
+   * The first of these matches Gradle's <a
+   * href="https://docs.gradle.org/8.11.1/dsl/org.gradle.api.artifacts.dsl.DependencyHandler.html#N1739F">
+   * external dependencies</a> form, and is the preferred format to use since it matches
+   * expectations of users of other tools. The second format is the one used within
+   * `rules_jvm_external` since its inception.
+   *
+   * <p>Note that there is potential confusion when only three segments are given (that is, the
+   * value could be one of `group:artifact:version` or `group:artifact:extension`) In this case, we
+   * assume the value is `group:artifact:version` as this is far more widely used.
+   */
   public Coordinates(String coordinates) {
     Objects.requireNonNull(coordinates, "Coordinates");
 

private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/ArtifactsHash.java

@@ -1,3 +1,17 @@
+// Copyright 2024 The Bazel Authors. All rights reserved.
+//
+// Licensed 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 com.github.bazelbuild.rules_jvm_external.resolver;
 
 import com.github.bazelbuild.rules_jvm_external.Coordinates;
@@ -6,21 +20,27 @@
 import java.util.TreeSet;
 import java.util.stream.Collectors;
 
-public class ArtifactsHash {
+/** Utility class for calculating artifact hashes for lock files. */
+public final class ArtifactsHash {
+
+  public static final int NO_HASH = -1;
+  private static final String SEPARATOR = " | ";
 
   private ArtifactsHash() {
     // utility class
   }
 
   public static int calculateArtifactsHash(Collection<DependencyInfo> infos) {
+    // Note: this function is exactly equivalent to the one in `v2_lock_file.bzl`
+    //       if you make a change there please make it here, and vice versa.
     Set<String> lines = new TreeSet<>();
 
     for (DependencyInfo info : infos) {
       StringBuilder line = new StringBuilder();
       line.append(info.getCoordinates().toString())
-          .append(" | ")
+          .append(SEPARATOR)
           .append(info.getSha256().orElseGet(() -> ""))
-          .append(" | ");
+          .append(SEPARATOR);
 
       line.append(
           info.getDependencies().stream()

private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/lockfile/V2LockFile.java

@@ -14,6 +14,7 @@
 
 package com.github.bazelbuild.rules_jvm_external.resolver.lockfile;
 
+import static com.github.bazelbuild.rules_jvm_external.resolver.ArtifactsHash.NO_HASH;
 import static com.github.bazelbuild.rules_jvm_external.resolver.ArtifactsHash.calculateArtifactsHash;
 import static com.google.common.base.StandardSystemProperty.USER_HOME;
 
@@ -179,7 +180,7 @@ public static V2LockFile create(String from) {
     }
 
     Number rawInputHash = (Number) raw.get("__INPUT_ARTIFACTS_HASH");
-    int inputHash = rawInputHash == null ? -1 : rawInputHash.intValue();
+    int inputHash = rawInputHash == null ? NO_HASH : rawInputHash.intValue();
 
     return new V2LockFile(inputHash, repos, infos, conflicts);
   }

tests/com/github/bazelbuild/rules_jvm_external/resolver/lockfile/V2LockFileTest.java

@@ -14,6 +14,7 @@
 
 package com.github.bazelbuild.rules_jvm_external.resolver.lockfile;
 
+import static com.github.bazelbuild.rules_jvm_external.resolver.ArtifactsHash.NO_HASH;
 import static org.junit.Assert.assertEquals;
 
 import com.github.bazelbuild.rules_jvm_external.Coordinates;
@@ -45,7 +46,8 @@ public void shouldRenderAggregatingJarsAsJarWithNullShasum() {
             Set.of(),
             new TreeMap<>());
 
-    Map<String, Object> rendered = new V2LockFile(-1, repos, Set.of(aggregator), Set.of()).render();
+    Map<String, Object> rendered =
+        new V2LockFile(NO_HASH, repos, Set.of(aggregator), Set.of()).render();
 
     Map<?, ?> artifacts = (Map<?, ?>) rendered.get("artifacts");
     Map<?, ?> data = (Map<?, ?>) artifacts.get("com.example:aggregator");
@@ -58,7 +60,7 @@ public void shouldRenderAggregatingJarsAsJarWithNullShasum() {
 
   @Test
   public void shouldRoundTripASimpleSetOfDependencies() {
-    V2LockFile roundTripped = roundTrip(new V2LockFile(-1, repos, Set.of(), Set.of()));
+    V2LockFile roundTripped = roundTrip(new V2LockFile(NO_HASH, repos, Set.of(), Set.of()));
 
     assertEquals(repos, roundTripped.getRepositories());
     assertEquals(Set.of(), roundTripped.getDependencyInfos());
@@ -67,7 +69,7 @@ public void shouldRoundTripASimpleSetOfDependencies() {
 
   @Test
   public void shouldRoundTripM2Local() {
-    V2LockFile lockFile = new V2LockFile(-1, repos, Set.of(), Set.of());
+    V2LockFile lockFile = new V2LockFile(NO_HASH, repos, Set.of(), Set.of());
     Map<String, Object> rendered = lockFile.render();
     rendered.put("m2local", true);
 
@@ -90,7 +92,7 @@ public void shouldRoundTripASingleArtifact() {
             Set.of(),
             new TreeMap<>());
 
-    V2LockFile lockFile = roundTrip(new V2LockFile(-1, repos, Set.of(info), Set.of()));
+    V2LockFile lockFile = roundTrip(new V2LockFile(NO_HASH, repos, Set.of(info), Set.of()));
 
     assertEquals(Set.of(info), lockFile.getDependencyInfos());
   }
@@ -119,7 +121,7 @@ public void shouldRoundTripASingleArtifactWithADependency() {
             Set.of(),
             new TreeMap<>());
 
-    V2LockFile lockFile = roundTrip(new V2LockFile(-1, repos, Set.of(info, dep), Set.of()));
+    V2LockFile lockFile = roundTrip(new V2LockFile(NO_HASH, repos, Set.of(info, dep), Set.of()));
 
     assertEquals(Set.of(info, dep), lockFile.getDependencyInfos());
   }
@@ -133,7 +135,7 @@ public void shouldRoundTripConflicts() {
             new Conflict(
                 new Coordinates("com.foo:bar:1.2.3"), new Coordinates("com.foo:bar:1.2.1")));
 
-    V2LockFile lockFile = roundTrip(new V2LockFile(-1, repos, Set.of(), conflicts));
+    V2LockFile lockFile = roundTrip(new V2LockFile(NO_HASH, repos, Set.of(), conflicts));
 
     assertEquals(conflicts, lockFile.getConflicts());
   }

tests/integration/maven_bom/BUILD

@@ -1,6 +1,5 @@
-load("@bazel_skylib//rules:diff_test.bzl", "diff_test")
 load("@bazel_skylib//rules:common_settings.bzl", "string_flag")
-load("@rules_java//java:defs.bzl", "java_library")
+load("@bazel_skylib//rules:diff_test.bzl", "diff_test")
 load("//:defs.bzl", "artifact", "java_export", "maven_bom")
 
 maven_bom(

tests/unit/exports/BUILD

@@ -1,4 +1,3 @@
-load("@rules_java//java:defs.bzl", "java_library")
 load("//tests/unit/exports:exports_test.bzl", "exports_tests")
 
 exports_tests(name = "exports_tests")