Inconsistent `VerseRef` creation behavior

[Enkidu93]

You can create a verse ref whose ToString() is LUK 1:1, by setting VerseRef.Verse but new VerseRef("LUK 1:1,") throws an exception. VerseRef LUK1:1,'s AllVerses() includes LUK 1: which is not valid. It seems to me that either setting Verse to LUK 1:1, should throw an exception or LUK 1:1, should be understood as synonymous with LUK 1:1 not LUK 1:1, LUK 1:. What's the intended behavior? Happy to put in a PR to fix this if it'd be helpful.

I discovered this issue while investigating sillsdev/serval#796.

[tombogle]

@FoolRunning Any idea whether this was/is intentional (seems unlikely) or whether fixing it one way or another would break something in Paratext? My inclination would be to say that setting Verse to LUK 1:1, should throw an exception, unless there are other cases where we try to be robust and trim off garbage. But to me it feels like it would be too hard to be sure that the trailing comma is just cruft that can be safely ignored as meaningless.

[Enkidu93]

Any updates on this?

[FoolRunning]

Sorry, I completely forgot about this.
I doubt it is intentional, and it probably just happened to be that way.
It's possible it has something to do with the way Paratext processes the text and it needed to allow invalid verses, but I doubt it.

[Enkidu93]

Sorry, I completely forgot about this. I doubt it is intentional, and it probably just happened to be that way. It's possible it has something to do with the way Paratext processes the text and it needed to allow invalid verses, but I doubt it.

No worries! Thank you - how could I go about addressing this? I believe that our USFM parser in Machine was modeled on or even copied from Paratext (is that right, @ddaspit?) and we do hit this situation, so maybe changing the behavior to throw an exception would break something in Paratext?

In the meantime, we've found a workaround on our end but it's a little cumbersome.

[tombogle]

If @FoolRunning or someone on the Paratext 9 team can work with us to follow up and ensure that fixing this doesn't create a subtle bug (or even a really obvious one) in Paratext, I think it's easy enough to fix the problem.