symposion / roll20-shaped-scripts

Support script for 5e Shaped Character Sheet on Roll20.net
http://roll20.net/
7 stars 5 forks source link

Support proper apostrophe #503

Closed mlenser closed 7 years ago

mlenser commented 7 years ago

I recently updated all the data to use proper apostrophes (’), but if I now try to import "Evard's Black Tentacles" instead of "Evard’s Black Tentacles" it won't work.

We should convert all straight apostrophes to proper typography apostrophes when searching, or allow it to search for either. If you use regex you can use:

const searchString = searchString.replace('\'', ('|’));

thorsteneb commented 7 years ago

I have this "mostly", and am giving up on finding what I'm missing.

Spell import with "typography" apostrophe or a single quote is working. I am forcing the "key" to use single quotes, and am converting an apostrophe to a single quote when searching for the spell to import.

Monster traits, actions, etc that have an apostrophe are working.

What's not working is correctly assigning renamed spells to a class after the fact. Example:

Artificer is listed as having "Leomund's Secret Chest". When "Leomund's Secret Chest" replaces SRD's "Secret Chest", that spell is NOT flagged for Artificer after the fact. I'm not understanding how those lists elements are built, and so I didn't manage to adapt that part of the code to apostrophes.

Workaround for now before I go bonkers: List Artificer as having "Secret Chest". When this is replaced by "Leomund's Secret Chest", it remains an Artificer spell. That also has the advantage that people that have SRD only can now see that spell.

I am 100% certain that the code I stuck in to make this "mostly work" is not graceful, and that I am missing parts that need to know about apostrophe. I am also certain, after banging my head against it for the better part of a day, that I am not the person to understand the code deeply enough to do better at this point in time.

This code in particular can benefit from further testing.

symposion commented 7 years ago

Urgh, this is a horrible thing to have to fix. You're trying to "auto-replace" quotes in a system where you only have control over a small part of it. I suspect you've only just scratched the tip of the iceberg with the bugs you're going to find. You need to think about, amongst others:

  1. All the code that finds existing spells/monsters when someone looks to import a new spell to make sure there isn't one there already. What about existing spells that have already been imported? Are you going to change them?
  2. Data files from outside - will the system cope if people choose not to use smart apostrophes in their data?
  3. Statblock import - currently I think it normalises everything to simple quotes to avoid problems here. I have spent the last year educating people not to use extended characters in their statblocks to avoid problems!
  4. I have a feeling that there's more than one unicode code point for typography apostrophes - and people on windows systems will probably throw horrible MS non-standard bullshit at you

That's just the stuff that occurs to me off the top of my head. I can pretty much guarantee that you will be fixing hard-to-find bugs with this for months to come. Personally it seems like a huge amount of effort for an almost invisible visual change; speaking as a programmer I'd like to find the guy who decided to introduce the concept of "smart quotes" into MS Word all those years ago and explain to him just how much pain and suffering he has caused; preferably with thumbscrews ;-)

If you're determined to change this then I think you will need to identify every point where data can enter into the system and replace at those points, rather than trying to fix individual bits of functionality, which will inevitably lead to bugs. I'd suggest looking at putting some stuff into:

  1. command-parser.js (which handles basically all the command-line input)
  2. entity-lookup.js (which handles loading all the data from data files)
  3. sanitise.js (which handles massaging the text for statblock import)

Try and get the replacement in as "early" as possible to avoid confusing anything else in the system.

F*cking typography weenies. Sigh. :-)

mlenser commented 7 years ago

Re #4: there is only 1 apostrophe character, nothing special here.

I believe Thorsten is using old data and that's part of the reason he's having trouble with cases like leomund.

I mention it on his fork, but the implementation he uses above seems to be going about it backwards

thorsteneb commented 7 years ago

1) For monsters, I'd have to test with one that has a single quote in its name. Can you think of one like that? For spells, right now, it will create a duplicate, and the spell isn't marked as existing in the spell explorer. 2) Yes, that continues to work. I've tested import of spells with single quotes. 3) I haven't changed much for monster import, just allowed apostrophes in mmFormatSpec.json. Single quote import should continue to work as before. 4) Please God No.

I can pretty much guarantee that you will be fixing hard-to-find bugs with this for months to come.

Or rather, hard-to-find bugs will elude me. I feel strongly that this is more than I bargained for when I started fixing small issues here and there. I am "barely holding on" when it comes to understanding what the code does, and what the data structures look like. A lot of the code is just plain beyond me, as shown by the problem with "Otiluke's Resilient Sphere" yesterday. It didn't show as an Artificer spell, because it wasn't marked as one when "Otiluke's Resilient Sphere" patched "Resilient Sphere". I side-stepped this by changing the Artificer entry to "Resilient Sphere", which also has the advantage that Artificer now shows that spell with SRD-only data. But it was a side-step. I was not able to understand where and how the class lists are built for spell entities.

Try and get the replacement in as "early" as possible to avoid confusing anything else in the system.

Right now I have a change in entity-lookup.js when adding entities (normalize key to single quotes, name stays whatever it is) and when searching for keys for import. Clearly I'm missing something with regards to "does the spell already exist".

F*cking typography weenies

Amen.

Forge ahead if you must, but understand: I am not a programmer. I last touched code on an Apple IIgs when putzing around with small command-line things in C 20 years ago, and I haven't ever worked with anything that has this level of abstraction with regards to how data structures are manipulated. I'm the monkey banging on a typewriter when it comes to Lucian's code. That will have consequences for what I can and can't do when bugs around apostrophe handling crop up.

mlenser commented 7 years ago

I'm going to tackle the apostrophe. I think I can handle it properly.

thorsteneb commented 7 years ago

Okay. As for #1, I have it showing in the spell explorer now as it should (when a single-quote spell exists and the data uses apostrophes), but import still brings in a duplicate.

thorsteneb commented 7 years ago

Duplicate on import also solved. I'll push, though it's an "incomplete" commit because there are some changes re spell uses in there. I think this covers the spell cases. I can't think of a monster with a single quote in its name to test there, that's likely broken for duplicate detection the same way spells were.

mlenser commented 7 years ago

Resolved as part of 11.0.0