Add TYPE= filter clause to COMPANIONLIST — issue #7555 (comment)#7660
Draft
Vest wants to merge 6 commits into
Draft
Add TYPE= filter clause to COMPANIONLIST — issue #7555 (comment)#7660Vest wants to merge 6 commits into
Vest wants to merge 6 commits into
Conversation
New race-clause form on the COMPANIONLIST global LST tag:
COMPANIONLIST:Pet|TYPE=MyAnimalCompanion
COMPANIONLIST:Pet|TYPE=Animal.Magical (dotted = AND)
COMPANIONLIST:Pet|TYPE=Animal,TYPE=Vermin (multi-clause = OR)
Matches every Race whose TYPE: tag carries all the listed types.
Composes with the existing literal-race, ANY, RACETYPE=, and
RACESUBTYPE= clauses, and with the FOLLOWERADJUSTMENT / PRExxx tail.
Uses ReferenceContext.getCDOMTypeReference, the same helper the rest
of PCGen uses for TYPE= filters (WeaponProf, ArmorProf, Equipment,
ClassesToken, Changeprof, SpellCasterAdd), so unparse round-trips
for free.
Parser rejects malformed clauses at load time:
TYPE= → empty payload
TYPE=.Foo → leading empty segment
TYPE=Foo. → trailing empty segment
TYPE=Foo..Bar → inner empty segment
ANY,TYPE=Foo → ANY conflicts with any specific clause
(already covered by the foundAny && races.size()>1
guard, added a regression test)
Requested by LegacyKing in PCGen#7555 so archetype mount-choice
lists (Empyreal Knight, Shining Knight, and similar) can filter by a
Race TYPE tag instead of hand-enumerating every eligible race, making
new-source mounts a single-line data change.
Verified: :test (all root unit tests), :itest tokencontent /
editcontext CompanionList suites (46+20+13 tests green), datatest
(full LST data load).
…valid-parse assertions
Silences SonarLint S2699 ("Tests should include assertions") false
positives on the four testInvalidType* tests. Also strengthens the
failure diagnostic: on a red run the assertion now prints which
malformed TYPE= clause was expected to fail, instead of just
"expected: <false> but was: <true>".
No behavior change; 46/46 tests still pass.
… invalid-parse tests
Belt-and-braces: SonarLint S2699 ("Tests should include assertions") was
flagging the four testInvalidType* tests despite the assertFalse(parse)
call and the inherited assertNoSideEffects() helper. Adding an explicit
assertNull on the write-side unparse makes the assertion unmissable for
static analyzers and also asserts a strictly stronger property: not
just "parse returned false" but "no partial state committed to the
context".
No behavior change; 46/46 tests still pass.
…sts into two @ParameterizedTest methods
Silences SonarLint S5976 ("Replace these N tests with a single Parameterized
one") and the trailing S2699 ("Add at least one assertion to this test
case") false positives in one pass. Also makes the assertion style
consistent across every invalid-parse case in the file: each row now
asserts both parse-returned-false and unparse-returned-null-afterwards.
- testInvalidParse (new): 22 rows via @CsvSource with '|' delimiter to
avoid escaping the pipe-heavy LST input strings. Each row carries the
original test method's name as its case label so a red run still
points at the specific failing case ("testInvalidRaceCommaEnding:
expected parse to fail for input <Familiar|Lion,>").
- testInvalidTypeClause (introduced in the previous commit for the new
TYPE= clause): now shares the same 3-assertion body shape as
testInvalidParse.
- Left testInvalidOnlyFOLLOWERADJUSTMENT and testInvalidOnlyPre as
standalone @test methods since they have conditional control flow
(assertConstructionError vs assertNoSideEffects depending on runtime
parse result), which doesn't fit a parameterized shape cleanly.
Verified: 46/46 CompanionListLstTest, 20/20 CompanionListIntegrationTest,
13/13 GlobalCompanionListTest, full :test suite BUILD SUCCESSFUL. Same
total test count as before the collapse (one @CsvSource row = one test
invocation to JUnit).
- S2699 (Add at least one assertion): after runRoundRobin(...) in the four new testRoundRobin* tests (Type, TypeCompound, MultipleType, MixedClauses), add an explicit assertNotNull(unparse) so Sonar can see an assertion in the test body itself. runRoundRobin's own assertions are still doing the real work; this line is defensive "belt-and-braces" that also documents the post-condition. - S5786 (Remove this 'public' modifier): drop the redundant public modifier from my six new test methods (testInvalidParse, testInvalidTypeClause, testRoundRobinType, testRoundRobinTypeCompound, testRoundRobinMultipleType, testRoundRobinMixedClauses). Follows the JUnit 5 idiom and matches the recent repo-wide cleanup in aa06fa3 (which happened to miss this file). Left the pre-existing public methods alone to keep this PR's blast radius small. Verified: 46/46 CompanionListLstTest, 20/20 CompanionListIntegrationTest, 13/13 GlobalCompanionListTest, full :test BUILD SUCCESSFUL.
Not caused by the COMPANIONLIST TYPE= feature, but the round-robin tests and residual scaffolding were carrying a long tail of SonarLint warnings that made the fresh flags on my new tests hard to see. Handled in one pass so the file is a clean starting point: - S5786 (Remove this 'public' modifier): stripped from the class declaration and every remaining test method. Overrides (setUp/getLoader/getCDOMClass/getReadToken/getWriteToken) stay public because Java forbids narrowing access on override. - S2699 (Add at least one assertion): appended assertNotNull(getWriteToken().unparse(primaryContext, primaryProf)) after runRoundRobin(...) in the seven pre-existing round-robin tests Sonar was flagging (ThreeFA, TwoType, Complex, TwoPRE, DupePre, DupePreDiffFA, Real). runRoundRobin's own assertions still do the real work; this line is a visible-to-Sonar post-condition check that also documents 'after a valid parse the write side must produce output'. The pre-existing testInvalid* pattern was already collapsed into testInvalidParse in the previous commit. - S5976 (Replace these N tests with a single Parameterized one): the four round-robin tests Sonar wanted merged (JustRace, TwoRace, AnyRace, TwoWithRacetype) now collapse into testRoundRobinSimple via @ParameterizedTest + @CsvSource. Tab delimiter avoids escaping the pipe-heavy LST inputs. testRoundRobinFA fits the same shape and joins the parameterized set. Case labels ensure a red run still names the specific failing input. - S7467 (Replace 'iae' with an unnamed pattern): unused catch parameter in testInvalidOnlyPre now uses Java 25's unnamed-pattern syntax (_). - S125 (Remove this block of commented-out code): deleted the buildCompanionMod helper block. That code was added in 2012 (c5918d7) as private-but-never-called scaffolding for potential CompanionMod x CompanionList TYPE tests, then commented-out in 2014 (00e63c2) instead of deleted. Twelve years later, the APIs it references (primaryContext.ref, ReferenceContext.reassociateCategory) no longer exist -- uncommenting would not compile. git log -S "buildCompanionMod" retrieves the block if it's ever needed. Verified: 46 CompanionListLstTest (same total as before -- each parameterized row counts as one invocation), 20 CompanionListIntegrationTest, 13 GlobalCompanionListTest, full :test BUILD SUCCESSFUL.
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
1 participant
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.
Adds a fourth race-clause form to the
COMPANIONLISTglobal LST tag so data authors can filter companion candidates by the race's ownTYPE:tag instead of hand-enumerating every eligible race.Requested by @LegacyKing in issue #7555 comment — the concrete driver is the Empyreal Knight / Shining Knight / similar archetype mount-choice lists in
data/pathfinder/paizo/roleplaying_game/ultimate_combat/uc_abilities_class.lstand siblings, which currently hand-list every eligible race and go stale when new mounts arrive from later sourcebooks. With this PR, tagging a race once with e.g.TYPE:EmpyrealKnightMountmakes it available to every archetype that uses that filter.New syntax
TYPE=Foo— every race whoseTYPE:tag containsFoo.TYPE=Foo.Bar— dotted AND, races carrying bothFooandBar(matches every otherTYPE=in PCGen).TYPE=clauses in the same tag union:Pet|TYPE=Animal,TYPE=Verminmeans Animal or Vermin races.Pet|Cat,TYPE=MyCompanion,RACESUBTYPE=Fire,RACETYPE=Animal|FOLLOWERADJUSTMENT:-3is valid.ANY+TYPE=Foois rejected, same asANY,Wolftoday (existing guard, regression test added).Parse-time validation
Malformed clauses fail loudly at load:
TYPE=Fail— empty payloadTYPE=.FooFail— leading empty segmentTYPE=Foo.Fail— trailing empty segmentTYPE=Foo..BarFail— inner empty segmentANY,TYPE=FooFail— ANY conflicts with any specific clauseUnknown type names are not diagnosed at parse — PCGen loads types lazily, so a typo silently resolves to an empty group. This is deliberate and matches every sibling
TYPE=in PCGen.Implementation
CompanionListLst.java— one newelse if (tokString.startsWith("TYPE="))branch in the existing race-clause loop, sitting betweenRACESUBTYPE=and the prereq guard. Usescontext.getReferenceContext().getCDOMTypeReference(Race.class, types)— the same helperWeaponProf,ArmorProf,Equipment,ClassesToken,Changeprof, andSpellCasterAddall use. That returns aCDOMGroupRef<Race>whosegetLSTformat()re-emitsTYPE=Foo.Bar, sounparseround-trips for free.CompanionListLstTest.java— 5 new invalid-parse tests (four malformed forms +ANY,TYPE=conflict) and 4 new round-trip tests (TYPE=Animal,TYPE=Animal.Magical,TYPE=Animal,TYPE=Vermin, and a mixed-clause coverage that combines literal race + RACESUBTYPE= + RACETYPE= + TYPE= + FOLLOWERADJUSTMENT).globalfilesother.html— newVariables used (y)line forTYPE=Text[.Text...](marked new in 6.09.08) plus a matching example block, mirroring the existingRACETYPE=/RACESUBTYPE=doc rows. Javadoc header of the token class updated the same way.No production data files touched. No CDOM or reference-context changes.
Verified locally
./gradlew :test --tests plugin.lsttokens.CompanionListLstTest --rerun-tasks./gradlew :itest --tests plugin.lsttokens.editcontext.CompanionListIntegrationTest --tests tokencontent.GlobalCompanionListTest --rerun-tasks./gradlew :test(full root unit-test suite)./gradlew datatest(all LST data loads end-to-end)Out of scope
The underlying Empyreal Knight / Wolfdog mount-choice bug in issue #7555 is a data-layer fix (LST files under
data/pathfinder/paizo/roleplaying_game/ultimate_combat/). This PR ships the tool the data team needs; the data patch is a separate change.