.MOD on Spells are CASE Sensitive to the point of Unconstructed errors if not exact.

Description

In debugging spells, I've come across an annoyance. Spell Names are required to be an Exact Match when it comes to 'Case'. Otherwise, you'll get an unconstructed reference error.

Cheers,

Environment

None

Activity

Show:
Andrew Maitland
April 25, 2016, 3:31 PM

As a side note:

In order for Data to work around this, we need to standardize to an Editor friendly method - which is lower case, UPPER CASE, or Mixed Case. Mixed case messes up roman numerals, UPPER case seems a bit overboard, and lower case doesn't look so great. I spent hours trying to implement a standard, and that breaks unit tests, which led to a migration patch, which doesn't work.

I think removing this restriction, bypasses data wasting time jumping through rules that don't need to exist.

Stefan Radermacher
April 25, 2016, 3:35 PM

I agree 100%.

Connor Petty
April 25, 2016, 4:40 PM

If case sensitivity is hurting development time I think we should move away from it. Trying to make it case insensitive might have some performance hit, but that would need to investigated. But regardless, It makes no sense to allow confusion or even a distinction between KeyName and KEYNAME, doing so causes more trouble than it is worth especially in LST files which are supposed to be easy to work with for non-programmers.

Stefan Radermacher
April 25, 2016, 11:47 PM

#1173

Stefan Radermacher
April 25, 2016, 11:58 PM

Spells use their own loader class (pcgen.persistence.lst.SpellLoader) while most other objects use a generic loader (pcgen.persistence.lst.GenericLoader). The generic loader stores the data in the reference context while the spell loader uses a global map.

I recently complained on the mailing list that I was no longer able to override spells with a newer version, and Tom Parker remarked this on the issue:

Spells have long been loaded differently than other objects. At quick glance, it looks like a change some years ago now (2 years, 10 months to be somewhat precise) broke how the Global list of spells was built and thus broke how spells are handled. This places them in a hybrid mode that is neither fully supported by the contexts (which are very much overwrite aware) nor supported by allowing multiple spells with the same key (and having the correct overwrite provisions)

Given that multiple spells with the same key can no longer work properly (and I now understand why Andrew made some of the data changes he made), I will look at simply doing a swap to where Spells are treated as any other object - and will thus inherit the correct override behavior. I certainly can't make it worse, and it's been something we wanted to do anyway... and the long lasting bug saves me the trouble of doing it via a code control...

So, on a hunch, I did a very simple change using GenericLoader instead of SpellLoader for spells, expecting that it might even solve the override problem. On first inspection, spells seem to load fine with the generic loader, and are case-insensitive for MODs, but the override issue remained. I will explore this further.

In the meantime I have changed the spell loaders global map to use a TreeMap with the String.CASE_INSENSITIVE_ORDER comparator, so spells look-ups for MODs are now case-insensitive.

Fixed

Assignee

Stefan Radermacher

Reporter

Andrew Maitland

Labels

None

Theme

None

Epic/Theme

None

Pending User Input

None

Fix versions

Affects versions

Priority

Minor
Configure