refactor(model): add pure-Java domain value types with TypedString/StringId typed IDs#13
Merged
Merged
Conversation
added 5 commits
July 9, 2026 15:58
Introduce `gg.steve.mc.ap.model` top-level package containing core domain value types organized by concept sub-package (set, ability, combat, effect, notification, player). All types are immutable value carriers using Lombok @Value/@builder with zero Bukkit/NMS/NBT imports. Types added: - model.set: PieceSlot enum, PurchaseResult enum - model.ability: AbilityType enum, BasicStats, HandItemSpec - model.combat: DamageContext, DamageModification - model.effect: EffectSpec, SoundSpec - model.notification: Notification - model.player: Wearer HandItemSpec carries the pure calculateFinalDamage math (mirroring HandSetData) and is added to the JaCoCo 100% core gate. All other types have construction/equality unit tests. Also adds: - Lombok 1.18.38 dependency (provided scope) with annotation processor - lombok.config enabling @generated annotation for JaCoCo exclusion - AGENTS.md updated with model package conventions
Rename all domain value types to use explicit, unambiguous names per project naming conventions: - PieceSlot -> ArmorPieceSlot - PurchaseResult -> ArmorSetPurchaseResult - AbilityType -> ArmorSetAbilityType - BasicStats -> ArmorSetBasicStats - HandItemSpec -> ArmorHandItemSpec - DamageContext -> CombatDamageContext - DamageModification -> CombatDamageModification - EffectSpec -> PotionEffectSpec - SoundSpec -> NotificationSoundSpec - Notification -> ArmorSetNotification - Wearer -> ArmorSetWearer Updates JaCoCo core gate include and AGENTS.md accordingly.
Use Lombok @Singular on messages and commands fields so the builder produces unmodifiable defensive copies at build() time. Callers retaining the original list can no longer mutate the notification post-construction. Tests added proving: - Mutating the source list after build does not affect stored value - getMessages()/getCommands() throw on modification attempts - Empty lists returned (not null) when no items added
… model collection fields
Rename PotionEffectSpec -> PotionEffect and NotificationSoundSpec -> NotificationSound. The clean short name belongs to the domain model; any future adapter code referencing the Bukkit PotionEffect uses the fully-qualified org.bukkit.potion.PotionEffect at that boundary.
Consistent with earlier rename of effect types - the domain model uses clean names without the Spec suffix. Updates JaCoCo core gate, README, and AGENTS.md references.
added 4 commits
July 10, 2026 08:17
Add model.id sub-package with typed string/UUID wrappers to eliminate primitive obsession for domain identity keys: - PlayerId (wraps UUID) - ArmorSetId (wraps String set name) - DamageCauseId (wraps String damage cause key) - PotionEffectId (wraps String effect type name) - SoundId (wraps String sound name key) Each provides a static of() factory. All model value types now use these typed IDs instead of raw String/UUID for identity fields. Free-text strings (messages, commands) remain unwrapped. Also renames ArmorHandItemSpec -> ArmorHandItem (drops Spec suffix consistent with prior effect type renames).
Add Lombok @nonnull to the value field in all typed ID wrappers so both the of() factory and the generated constructor reject null. This closes the bypass where new ArmorSetId(null) would silently succeed. The of() factory now simply delegates to the constructor (Lombok handles the null check). Update AGENTS.md to reference typed ID wrappers as the identity convention.
added 5 commits
July 10, 2026 08:59
Replace per-wrapper Lombok @value IDs with a shared base-class hierarchy using commons-lang3 for equals/hashCode/validation: - TypedString: abstract base with final String id field, null-rejecting constructor (Validate.notNull), EqualsBuilder/HashCodeBuilder, and a static toStrings() utility - StringId extends TypedString: adds Validate.notEmpty guard - ArmorSetId, DamageCauseId, PotionEffectId, SoundId extend StringId - PlayerId extends TypedString (stores UUID, exposes getValue()) Adds commons-lang3 3.17.0 as a compile dependency, shaded under gg.steve.mc.ap.lib.commons.lang3. TypedString and StringId added to the JaCoCo 100% core gate.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Intent
Refactor typed ID wrappers from standalone Lombok @value classes to a shared TypedString/StringId base-class hierarchy using commons-lang3 for equals/hashCode/validation. TypedString is the abstract base with null-rejecting constructor, HashCodeBuilder/EqualsBuilder (with getClass() check so different subtypes with same string are NOT equal), toString(), and a static toStrings() utility. StringId extends TypedString adding Validate.notEmpty. ArmorSetId, DamageCauseId, PotionEffectId, SoundId extend StringId with private constructor + of() factory. PlayerId extends TypedString directly (wraps UUID, stores UUID.toString() for the base, exposes getValue() for the original UUID). commons-lang3 3.17.0 added as compile dep, shaded to gg.steve.mc.ap.lib.commons.lang3. TypedString and StringId added to 100% JaCoCo core gate. The captain provided the exact TypedString source shape to implement faithfully. DamageCauseId is a bounded domain key (not free text), confirmed as correct by captain.
What Changed
gg.steve.mc.ap.modelpackage of pure-Java domain value types (set, ability, combat, effect, notification, player sub-packages) built with Lombok@Value/@Builderand@Singularfor immutable collections, with zero Bukkit/NMS/NBT imports.ArmorSetId,DamageCauseId,PotionEffectId,SoundId,PlayerId) on a sharedTypedString/StringIdbase hierarchy: null/empty rejection via commons-lang3Validate,getClass()-checked equals/hashCode so different subtypes wrapping the same string are never equal, final leaf classes with private constructors andof()factories; commons-lang3 3.17.0 is added as a compile dependency and shaded togg.steve.mc.ap.lib.commons.lang3.check-core100% line+branch gate to coverTypedStringandStringId, added unit tests for every new model type (185 tests total pass undermvn -B clean verifyon JDK 25), gitignoredbuild.log, and updated AGENTS.md/README.md conventions to match the new hierarchy.Risk Assessment
✅ Low: Additive-only change introducing pure-Java value types with comprehensive test coverage and a 100% JaCoCo gate; no existing behavior is altered, and all previous review findings have been resolved.
Testing
Full mvn verify under JDK 25 confirms the TypedString/StringId hierarchy works as intended: all 185 tests pass green, the 100% coverage gate on the two abstract bases succeeds, commons-lang3 is fully shaded in the final plugin JAR, and cross-type inequality plus null/empty rejection are exercised by dedicated tests.
Evidence: Verification results (build, tests, JaCoCo, shading)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (2) ✅
src/main/java/gg/steve/mc/ap/model/id/ArmorSetId.java:3- The leaf ID classes (ArmorSetId, DamageCauseId, PotionEffectId, SoundId, PlayerId) are not declared final. The private constructors already prevent external subclassing, making them effectively final, but marking them final would communicate the design intent explicitly and prevent accidental internal subclassing within the same package.🔧 Fix: Mark leaf ID classes final
4 issues (2 warnings, 2 infos) still open:
build.log:1- Commit fbd5a11 re-commits the 146-line Maven build.log at the repo root, directly reverting the immediately preceding commit d766053 ("chore: remove accidental build log"). The file is a throwaway build artifact containing machine-specific absolute paths (/Users/goodhill/...). It should be deleted again, and adding abuild.logentry to .gitignore would stop this recurring (the agent workflow routinely redirects build output to build.log).AGENTS.md:15- AGENTS.md line 15 still documents the old Lombok-based design: "ID wrappers enforce null safety via @nonnull on the value field; both the generated constructor and the of() factory reject null". Commit 341947a replaced that with the TypedString/StringId hierarchy where null rejection happens via commons-lang3 Validate.notNull in the base constructor and there is no @nonnull or generated constructor anymore. The convention note should be rewritten to describe the TypedString/StringId base-class approach so future agent sessions don't follow stale guidance.AGENTS.md:8- pom.xml adds gg.steve.mc.ap.model.id.TypedString and StringId to the JaCoCo check-core 100% gate, but the documented core-class lists in AGENTS.md line 8 and README.md line 15 were not updated to include them (they stop at ArmorHandItem). Future contributors modifying TypedString/StringId won't know the build enforces 100% coverage on them.pom.xml:246- commons-lang3 3.17.0 (~670 KB) is compile-scoped and shaded into the plugin jar to supply just three small utilities (Validate, EqualsBuilder, HashCodeBuilder). The relocation to gg.steve.mc.ap.lib.commons.lang3 is correct for avoiding server classpath clashes, but the whole library lands in the artifact since minimizeJar is not enabled. Acceptable tradeoff for a Spigot plugin; noting it in case jar size ever matters.🔧 Fix: Delete build.log, gitignore it, update stale docs
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
mvn -B clean verify (JDK 25) - 185 tests, BUILD SUCCESS, JaCoCo check-core 100% line+branch on TypedString and StringIdVerified commons-lang3 shading: 413 classes at gg/steve/mc/ap/lib/commons/lang3/, 0 at org/apache/commons/lang3/ in final JARConfirmed cross-type inequality test (TypedStringTest.differentSubtypesWithSameStringAreNotEqual)Confirmed null/empty rejection tests (TypedStringTest.nullConstructorThrows, StringIdTest, PlayerIdTest)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.