Use Guava for Preconditions and Verify #114
Conversation
I'm a smidge reluctant to add an implementation dependency, but the Guava utilities are useful enough to make it worthwhile.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces Guava as a new dependency to replace the custom Invariants utility, migrating assertion logic to Guava’s Preconditions and Verify, and updating build/configuration files accordingly.
- Add Guava dependency and replace custom null‐checks and invariant throws with
Preconditions.checkArgument,checkNotNull, andVerify.verify. - Remove the
Invariantsclass and deprecatedisForExpiredElements()methods, updating safety checks across lists andInnerArc. - Update module‐info, Gradle setup, and Checkstyle rules to include Guava and ban legacy nullness annotations.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/main/java/module-info.java | Added requires com.google.common module |
| lib/src/main/java/eu/aylett/arc/internal/UnownedElementList.java | Removed unused override of isForExpiredElements() |
| lib/src/main/java/eu/aylett/arc/internal/LRUElementList.java | Replaced manual IllegalStateException checks with verify |
| lib/src/main/java/eu/aylett/arc/internal/ExpiredElementList.java | Swapped manual throws for Guava verify/checkArgument |
| lib/src/main/java/eu/aylett/arc/internal/Invariants.java | Deleted obsolete custom assertions class |
| lib/src/main/java/eu/aylett/arc/internal/InnerArc.java | Imported Guava verify, added unowned.checkSafety() call |
| lib/src/main/java/eu/aylett/arc/internal/ElementList.java | Removed abstract isForExpiredElements() |
| lib/src/main/java/eu/aylett/arc/internal/Element.java | Switched to JSPECIFY @NonNull & Guava verify in get() |
| lib/src/main/java/eu/aylett/arc/Arc.java | Swapped argument checks to Guava checkArgument/checkNotNull, added Javadoc for runEviction |
| lib/build.gradle.kts | Changed several dependencies to api and added implementation(libs.guava) |
| gradle/libs.versions.toml | Added Guava version and module entries |
| config/checkstyle/checkstyle.xml | Introduced IllegalImport rule for legacy nullness annots |
Comments suppressed due to low confidence (2)
lib/src/main/java/eu/aylett/arc/internal/LRUElementList.java:89
- [nitpick] The variable name
startSizediffers frominitialSizeinExpiredElementList. Consider using a consistent name (e.g.,initialSize) across both implementations for clarity.
verify(seen.size() == startSize, "Size mismatch: found %s items != expected %s", seen.size(), startSize);
lib/src/main/java/eu/aylett/arc/internal/InnerArc.java:119
- New call to
unowned.checkSafety()was added but no tests have been updated. Consider adding unit tests to cover the safety checks for theUnownedElementList.
unowned.checkSafety();
| throw new IllegalStateException("Called get on an object in an expired list"); | ||
| } | ||
| verify(currentOwner != unowned, "Called get on an element with no owner"); | ||
| verify(currentOwner instanceof LRUElementList, "Called get on an object in an expired list"); |
There was a problem hiding this comment.
Relying on instanceof LRUElementList couples the check to a specific implementation. Consider restoring an explicit method (e.g., isForExpiredElements()) or adding a boolean flag in the ElementList hierarchy to detect expired lists more generically.
Copilot uses AI. Check for mistakes.
| @@ -223,6 +217,19 @@ public void checkSafety() { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
The runEviction method has a detailed Javadoc but lacks thread‐safety annotations (e.g., @GuardedBy) or clearer lock usage notes. Consider annotating which lock is held or required to align with the rest of the class’s conventions.
Copilot uses AI. Check for mistakes.
|
I'm a smidge reluctant to add an implementation dependency, but the Guava utilities are useful enough to make it worthwhile.