[Pcgen-github] [PCGen/pcgen] 9cf92c: RollInfo: fix two round-trip bugs and tighten the ...

[Vest <no...@gi...>]

  Branch: refs/heads/master
  Home:   https://github.com/PCGen/pcgen
  Commit: 9cf92cc07561c758894e5eed2400434eb20b0d16
      https://github.com/PCGen/pcgen/commit/9cf92cc07561c758894e5eed2400434eb20b0d16
  Author: Vest <Ve...@us...>
  Date:   2026-06-07 (Sun, 07 Jun 2026)

  Changed paths:
    M code/src/java/pcgen/core/Globals.java
    M code/src/java/pcgen/core/RollInfo.java
    M code/src/java/pcgen/core/term/EQDamageDiceTermEvaluator.java
    M code/src/java/pcgen/core/term/EQDamageDieTermEvaluator.java
    M code/src/test/pcgen/core/GlobalsTest.java
    A code/src/utest/pcgen/core/RollInfoTest.java

  Log Message:
  -----------
  RollInfo: fix two round-trip bugs and tighten the class (#7586)

* RollInfo: rewrite toString() keep-list rendering

The keep-list section used `while (keepList != null) // let break work`
with `break` as a goto-style label. The body never iterates — every
path either falls through or breaks. Extract it as a helper that returns
false on the malformed all-false case so toString() can bail.

* RollInfo: add copy constructor; collapse Globals.adjustDamage clone hack

Globals.adjustDamage cloned a RollInfo via a toString-then-reparse round
trip ("Ugh, have to do this for 'cloning' to avoid polluting the master")
and then mutated the clone's `times` field directly. The round trip is
both lossy (drift risk on toString/parse) and forces the field to stay
package-visible.

Add `RollInfo(RollInfo other, int timesMultiplier)` for explicit cloning
with a scale factor, and use it in Globals.adjustDamage.

* RollInfo: throw IllegalArgumentException on parse failure

Constructor previously logged the error and returned a zeroed object,
forcing every caller to re-check for "empty" RollInfo or trust unparseable
input. Now it throws, and the four direct callers handle the failure
explicitly:

- Globals.adjustDamage falls back to returning aDamage unchanged
- EQDamage{Dice,Die}TermEvaluator returns "0"

UpToken / DownToken already had IAE catch blocks (previously dead code);
they now actually fire on bad input.

Also clarifies the "Need to test for higher dice" comment in adjustDamage
and replaces a direct .sides field read with .getSides().

* RollInfo: tighten sides/times field access to private

The class is final and all reads now go through getSides() / getTimes()
or the copy constructor, so protected was misleading — it suggested a
subclass extension point that doesn't exist.

* RollInfo: add tests for parser, copy ctor, and adjustDamage fallback

- New RollInfoTest covers round-trip toString for the standard forms
  (NdM, modifiers, keep top/bottom/list, reroll bounds, total floor/ceiling),
  the copy constructor (incl. keepList cloning), validateRollString, and
  the IllegalArgumentException contract on bad input.
- GlobalsTest.testAdjustDamage gains two cases: NdM scaling via the 1dM
  step lookup, and the unparseable-input fall-through.
- RollInfo.parseRollInfo now also catches NoSuchElementException (raised
  by StringTokenizer on empty input) so the IAE contract is honoured.

* RollInfo: simplify toString helpers per Sonar feedback

- Merge the nested if at toString() / appendKeepList call site (S1066).
- Split appendKeepList into countKept / leadingKeptRun / trailingKeptRun
  helpers and emit the explicit list via IntStream + Collectors.joining
  (S3776: cognitive complexity 27 → under threshold).

* RollInfo: extract duplicated parser literals (Sonar S1192)

- "Bad roll parsing in '" → PARSE_ERROR_PREFIX
- "mM+-tT" → POST_SIDES_DELIMS (also reused in the initial "/\|mM+-tT"
  delimiter set)

* RollInfoTest: move to utest source set

The class has no Globals/GameMode/file-loader dependencies — it's a
pure unit test. Relocating from slowtest to utest puts it on every PR's
fast feedback loop and removes unnecessary JavaFX module-path overhead.

Pure rename, no content change.

* RollInfo: fix two round-trip bugs surfaced by tests

1. Parser ignored carefully-set parseChars in the modifier (+/-) and
   total-clamp (t/T) branches, calling nextToken(" ") and greedily eating
   the next clamp character. So '1d20+5t2' threw NumberFormatException on
   "5t2", and toString() could emit strings the parser refused — round-
   trip was broken for any modifier-then-clamp combo.

2. Copy constructor cloned keepList as-is even when timesMultiplier > 1.
   The cloned array was sized to the source's times, but this.times got
   scaled, so toString() walked past the end and threw
   ArrayIndexOutOfBoundsException. There's no canonical way to extend a
   keep-list across replicated dice groups, so scaling now drops it.

Regression tests added for both.

* RollInfoTest: parameterize round-trip, normalisation, and rejection cases

Collapse the per-notation round-trip tests, the bad-input rejection
tests, and a new validate-vs-constructor agreement check into
@ParameterizedTest blocks driven by @ValueSource and @CsvSource. Adds
coverage for the implicit-times-1 case (no leading number), keep-list
shape normalisation (|a,b -> /n or \n where contiguous), the
discontinuous-stays-explicit case, and the negative-sides rejection.

* RollInfo: correct constructor Javadoc to match parser

Two errors in the syntax doc:
1. m/M were described with their reroll fields swapped — the parser
   sets rerollBelow on 'm' and rerollAbove on 'M', not the reverse.
2. t/T described totalFloor/totalCeiling without saying they clamp the
   total in the named direction.

Also fixes typos ("postitive", "*integer").

* EQ damage evaluators: log unparseable damage instead of silently zeroing

The catches added when RollInfo started throwing IllegalArgumentException
were swallowing the original cause without trace. The old behaviour
(silent log + zeroed RollInfo) at least left a Logging.errorPrint
breadcrumb; the new catches threw that away.

Reinstate the diagnostic: log the offending equipment key and the
unparseable damage string before falling through to "0", so LST data
defects don't disappear into a silent zero.



To unsubscribe from these emails, change your notification settings at https://github.com/PCGen/pcgen/settings/notifications