search

sillsdev / scripture

TypeScript port of the 'libpalaso/SIL.Scripture' library that provides classes for working with Scripture data such as references and versifications.
MIT License
0 stars 1 forks source link

VerseRef constructor accepts invalid input #18

Open Nateowami opened 3 months ago

Nateowami commented 3 months ago

This throws an exception, as expected:

new VerseRef('1PE 3')

This does not throw an exception:

new VerseRef('1PE 3:6=&*(&^&*$%&^%^asdfoajsdfjalsdhfnjk56asdhf')
irahopkinson commented 3 months ago

Indeed, the first one throws an Invalid reference exception because the reference is incomplete without also specifying the verse.

For the second one it is less clear. You could argue that it is following the good practice of being tolerant of input since verseNum does get parsed to the number 6, however verse is "6=&*(&^&*$%&^%^asdfoajsdfjalsdhfnjk56asdhf". I agree that's not good overall.

Given that this is actually following the original behavior of the C#, I'm a little reticent to "fix" it here. Perhaps we should repost this issue to the C# repo? Is there an existing issue that fixing this in the TS version would help with or is this just messy behavior for now?

Nateowami commented 3 months ago

You could argue that it is following the good practice of being tolerant of input since verseNum does get parsed to the number 6, however verse is "6=&*(&^&*$%&^%^asdfoajsdfjalsdhfnjk56asdhf".

Correct. However, from my perspective this behavior is useless, since it hasn't actually parsed the verse range. I would like to be able to convert a verse ref into a list of discrete verses. If that can't actually be done, I don't think the software should accept it as a reference. Maybe there's a use-case I'm not aware of, but Scripture Forge would want to be able to highlight verses associated with a question, which is associated with a verse ref.

irahopkinson commented 3 months ago

Perhaps I'm missing something but 6 appears to be the only verse range that I can parse from "6=&*(&^&*$%&^%^asdfoajsdfjalsdhfnjk56asdhf". What is your expected outcome that VerseRef should do with this?

Nateowami commented 3 months ago

It seems to accept literally anything as a verse range, and then not parse the verse range, except to try to get the first number out of it. VerseRef.tryParse returns an object with success: true almost no matter how badly formed the verse range part is.

The only workaround for this I can think of is to take the verse range part, and then try to parse it by splitting on ,, -, and maybe a couple of other things, but that seems like something this library ought to do itself.