While working with spell MODs I noticed that some spell tags are additive lists, i.e. when used in a MOD, they create a list of the original value and the value that is intended to replace it, making it necessary to use .CLEAR to remove the original value.
The affected tags are CASTTIME, COMPS, DURATION, RANGE, SAVEINFO, and SPELLRES. It doesn't make any sense for these tags to support multiple values and thus to force using .CLEAR for MODs
The tags SCHOOL and SUBSCHOOL usually only have one value, too, but spells have been known to use multiple schools (there were some in late D&D v.3.5, but closed content). We should allow this, even though we have no way of associating the subschools with their schools in such a case. These tags should be derived from AbstractNonEmptyToken instead of AbstractTokenWithSeparator.
The tags DESCRIPTOR and VARIANTS should continue to take lists.
That leaves TARGETAREA which takes only one value, but seems to have an antiquated implementation using CDOMPrimaryToken, and should probably use AbstractNonEmptyToken, too.
I know that these tags are all destined to be replaced with INFO, but this has no fixed timeframe, so I'd like to clean this up anyway.
As an aside, I think it is a bad idea that when PCGen was originally designed, it was decided to conflate the Target, Area, and Effect lines of spells into the single TARGETAREA tag. While there is only ever one of these lines per spell, it can be necessary to know whether a spell is a target, area, or effect spell, because some effects interact only with targeted spells, or only with area spells, and so on. The way this is coded up, there is no way PCGen could indicate what kind a spell is on the output sheet.
Since the tag is going to be replaced by an INFO tag, I'm not going to suggest breaking it up now, but strongly suggest that, when the time to move to INFO comes, we try to find a way to distinguish between target, area, and effect spells.
(If I were to suggest changing it now, I'd propose changing the TARGETAREA tag so that it can alternatively take two parameters, so it could read TARGETAREA:Target|1 creature, or TARGETAREA:Effect|1 missile, but also still TARGETAREA:20-ft. radius. That way, a migration could be performed gradually. This approach will probably not work with a single INFO:TARGETAREA, so maybe three separate INFO tags would be best later on.)
However, the spell block will have to be dynamic in which field is
output. <fo:inline font-weight="bold"> TARGET: </fo:inline><xsl:value-of
Would need to be cleaned up to be:
<xsl:if test="string-length(target) > 0">
<fo:inline font-weight="bold"> TARGET: </fo:inline><xsl:value-of
Repeat for the other two variants with the name changing.
Easily done now actually since the system support lists. Time frame
aside, the only issue would be if any of those fields have a dynamic
output based upon the caster level. If we are only doing strict text,
then this is supported today.
I find it most distressing we have an "EFFECT" and "DESCRIPTION" tag,
but the spells use 'EFFECT" to output the DESC tag.. Counter-intuitive...
I think rather than changing the tokens (which changes behavior) we should simply deprecate most of the ones you list and change the data to use FACTs...
or perhaps in the case of COMPS, there are some other things we could do, as we will end up with enumerations as a data format at some point