Convert USFM versification

[Enkidu93]

Fixes #421.

There are still some changes we'll need in Serval to fully address this problem besides just utilizing this functionality (see sillsdev/serval#1027 for example).

---

This change is 

[pmachapman]

I don't think code handles the common use case of a verse jumping back and forth across chapters, i.e. for Original to English:

• GEN 32:1 to GEN 31:55
• ISA 8:23 to ISA 9:1

Here are two unit test (I think these are correct???) that fail for these cases:

    [Test]
    public void GetUsfm_ConvertUsfmToUpdateRowVersification_BackOneVerseToPreviousChapter()
    {
        // English vs. Original
        // ISA 9:1 = ISA 8:23

        List<UpdateUsfmRow> rows =
        [
            new UpdateUsfmRow(ScrRef(["ISA 9:1"], ScrVers.English), "Updated verse 1"),
            new UpdateUsfmRow(ScrRef(["ISA 9:2"], ScrVers.English), "Updated verse 2"),
        ];

        string usfm =
            @"\id ISA - Test
\c 8
\v 23
\c 9
\v 1
";

        string target = UpdateUsfm(
            rows,
            usfm,
            bookId: "ISA",
            convertUsfmToUpdateRowVersification: true,
            versification: ScrVers.Original
        );
        string result =
            @"\id ISA - Test
\c 8
\c 9
\v 1 Updated verse 1
\v 2 Updated verse 2
";
        AssertUsfmEquals(target, result);
    }

    [Test]
    public void GetUsfm_ConvertUsfmToUpdateRowVersification_ForwardOneVerseToNextChapter()
    {
        // Original vs. English
        // ISA 8:23 = ISA 9:1

        List<UpdateUsfmRow> rows =
        [
            new UpdateUsfmRow(ScrRef(["ISA 8:23"], ScrVers.Original), "Updated verse 23"),
            new UpdateUsfmRow(ScrRef(["ISA 9:1"], ScrVers.Original), "Updated verse 1"),
        ];

        string usfm =
            @"\id ISA - Test
\c 8
\c 9
\v 1
\v 2
";

        string target = UpdateUsfm(
            rows,
            usfm,
            bookId: "ISA",
            convertUsfmToUpdateRowVersification: true,
            versification: ScrVers.English
        );
        string result =
            @"\id ISA - Test
\c 8
\v 23 Updated verse 23
\c 9
\v 1 Updated verse 1
";
        AssertUsfmEquals(target, result);
    }

For GetUsfm_ConvertUsfmToUpdateRowVersification_BackOneVerseToPreviousChapter(), the value of target is:

\id ISA - Test
\c 8
\v 1 Updated verse 1
\v 2
\c 9 Updated verse 2

For GetUsfm_ConvertUsfmToUpdateRowVersification_ForwardOneVerseToNextChapter(), the value of target is:

\id ISA - Test
\c 8
\c 9
\v 23 Updated verse 23
\c 9
\v 1 Updated verse 1

(apologies if my tests are wrong or if I am misunderstanding this PR)

@pmachapman reviewed 4 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on ddaspit and Enkidu93).

---

src/SIL.Machine/Corpora/UsfmToken.cs line 183 at r1 (raw file):

        }

        public UsfmToken Copy()

Optional: If you make this:

public UsfmToken Clone()

Then this class could implement ICloneable<>:

using SIL.ObjectModel;

...

public class UsfmToken: IEquatable<UsfmToken>, ICloneable<UsfmToken>

Code quote:

public UsfmToken Copy()

---

tests/SIL.Machine.Tests/Corpora/UpdateUsfmParserHandlerTests.cs line 885 at r1 (raw file):

            convertUsfmToUpdateRowVersification: false,
            versification: ScrVers.RussianOrthodox
        );

NIT: Should this have the bookId specified?

        target = UpdateUsfm(
            rows,
            usfm,
            bookId: "PSA",
            convertUsfmToUpdateRowVersification: true,
            versification: ScrVers.RussianOrthodox
        );

Code quote:

        string target = UpdateUsfm(
            rows,
            usfm,
            convertUsfmToUpdateRowVersification: false,
            versification: ScrVers.RussianOrthodox
        );

---

tests/SIL.Machine.Tests/Corpora/UpdateUsfmParserHandlerTests.cs line 940 at r1 (raw file):

            convertUsfmToUpdateRowVersification: true,
            versification: ScrVers.Original
        );

NIT: Should this have the bookId specified?

        string target = UpdateUsfm(
            rows,
            usfm,
            bookId: "DAN",
            convertUsfmToUpdateRowVersification: true,
            versification: ScrVers.Original
        );

Code quote:

        string target = UpdateUsfm(
            rows,
            usfm,
            convertUsfmToUpdateRowVersification: true,
            versification: ScrVers.Original
        );

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.84979% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.66%. Comparing base (418ff22) to head (a50f3a5).

Files with missing lines	Patch %	Lines
...Machine/Corpora/ConvertUsfmVersificationHandler.cs	94.47%	5 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #472      +/-   ##
==========================================
+ Coverage   73.52%   73.66%   +0.13%     
==========================================
  Files         451      452       +1     
  Lines       37697    37926     +229     
  Branches     5183     5210      +27     
==========================================
+ Hits        27718    27939     +221     
- Misses       8841     8842       +1     
- Partials     1138     1145       +7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Enkidu93]

Thank you for catching this, Peter. It turns out that these test failures have less to do with the specific mapping scenario and more to do with whether the mapping verses are padded with verses etc. Goes to show that writing tests is harder than you think 🤪. After reviewing the problems, I've decided the simplest thing to do is to have the verse handler add all chapter tokens rather than trying to re-use the encountered ones when possible. The only downside with this is that empty chapters (after the conversion) will be stripped. I don't think this is unreasonable behavior though, and realistically, I don't think it will make a difference.

@Enkidu93 made 4 comments and resolved 2 discussions.
Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on ddaspit and pmachapman).

---

src/SIL.Machine/Corpora/UsfmToken.cs line 183 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Optional: If you make this:

public UsfmToken Clone()

Then this class could implement ICloneable<>:

using SIL.ObjectModel;

...

public class UsfmToken: IEquatable<UsfmToken>, ICloneable<UsfmToken>

Done. Thanks!

---

tests/SIL.Machine.Tests/Corpora/UpdateUsfmParserHandlerTests.cs line 885 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Should this have the bookId specified?

        target = UpdateUsfm(
            rows,
            usfm,
            bookId: "PSA",
            convertUsfmToUpdateRowVersification: true,
            versification: ScrVers.RussianOrthodox
        );

Done.

---

tests/SIL.Machine.Tests/Corpora/UpdateUsfmParserHandlerTests.cs line 940 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Should this have the bookId specified?

        string target = UpdateUsfm(
            rows,
            usfm,
            bookId: "DAN",
            convertUsfmToUpdateRowVersification: true,
            versification: ScrVers.Original
        );

Done.

[pmachapman]

from my perspective. I'm sure using it in Serval will throw all sorts of interesting edge cases we haven't thought of!

@pmachapman reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on ddaspit).

[ddaspit]

Trying to incorporate the conversion into the UpdateUsfmParserHandler adds a lot of complexity. To fix some of the issues that I raised, we will need to add lookback/lookahead to the parser handler. I think it would be better if we perform the conversion before updating. I'm pretty sure this can be done in a single pass over the USFM tokens. We could encapsulate the conversion in a new class, so that there is a proper separation of concerns. I can throw together a prototype if that would help explain better what I'm looking for.

@ddaspit reviewed 4 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on Enkidu93).

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 414 at r4 (raw file):

            if (!_skipNextVerseText)
            {
                EndUpdateBlock(state, scriptureRefs);

Do you need to do this for non-verse text and embeds?

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 415 at r4 (raw file):

            {
                EndUpdateBlock(state, scriptureRefs);
                _skipNextVerseText = false;

Is this in the right place?

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 585 at r4 (raw file):

                    if (updatedVerse.BookNum != state.VerseRef.BookNum)
                    {
                        _tokenIndex++;

What about the rest of the verse content? I think it will get added to the preceding element.

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 619 at r4 (raw file):

            if (_convertUsfmToUpdateRowVersification)
            {
                verseRef = verseRef.ChangeVersificationWithSegments(_updateRowsVersification);

What about verse ranges that map across chapters?

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 629 at r4 (raw file):

                        verseRef.ChapterNum.ToString()
                    );
                    _tokens.Add(newChapterToken);

If there are any preceding paragraph or section markers, the chapter marker will get added at the wrong place.

[Enkidu93]

Yeah, I agree. If it's any more complex than this, we ought to move it to a separate class. I'll take a stab at it.

@Enkidu93 made 1 comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on Enkidu93).

[Enkidu93]

I've gone ahead and moved the conversion code to a separate handler. Of course, the only downside to this is that we'll need to parse the USFM twice - once with each handler. I still want to add a couple tests covering how the code handles invalid verses references / chapter references and add checks as needed since this is something we see fairly frequently as well as File and Zip implementations, but how does this look to you, @ddaspit? Is this the sort of thing you had in mind?

Also, handling the cross-chapter verse ranges added a lot of complexity. I ended up making one code path in Verse() for both these verse ranges as well as normal verses. If you'd prefer it to branch and then move the cross-chapter handling to a separate method for clarity, I'm happy to do that too.

@Enkidu93 made 6 comments.
Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on ddaspit and pmachapman).

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 414 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Do you need to do this for non-verse text and embeds?

Removed.

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 415 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Is this in the right place?

Removed.

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 585 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

What about the rest of the verse content? I think it will get added to the preceding element.

Removed.

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 619 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

What about verse ranges that map across chapters?

Removed.

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 629 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

If there are any preceding paragraph or section markers, the chapter marker will get added at the wrong place.

Removed.

[ddaspit]

I'm okay with the extra pass and with verse range handling in Verse().

@ddaspit reviewed 8 files and all commit messages, made 5 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on Enkidu93).

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 68 at r5 (raw file):

                if (_insertChapterIndex == -1)
                    _tokens.Add(newChapterToken);

When we add a chapter marker in the middle of a paragraph, we should also add a paragraph marker. I think we want an \nb here (see new GetUsfm_SynthesizedChapter tests).

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 89 at r5 (raw file):

                    {
                        _tokens.Add(new UsfmToken(UsfmTokenType.Verse, "v", "", "", start + end));
                        _tokens.Add(new UsfmToken(UsfmTokenType.Chapter, "c", "", "", verseRefs[i].Chapter));

When we split a verse range across a chapter boundary, all of the verse's text ends up on the second half and the first half is left empty. I think we should put the text in the first verse (see new GetUsfm_SplitVerseRange_TextStaysWithFirstVerse test).

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 143 at r5 (raw file):

        {
            int offset = 0;
            if (!_skip)

When a verse is dropped for mapping into another book, the headings that follow it get dropped too (see the two new GetUsfm_HeadingIntroducing tests).

---

src/SIL.Machine/Corpora/ParatextProjectVersificationConverterBase.cs line 9 at r5 (raw file):

namespace SIL.Machine.Corpora
{
    public abstract class ParatextProjectVersificationConverterBase

Rather than creating a new converter base class, could we integrate the conversion directly into ParatextProjectTextUpdaterBase? Something like this should work:

IReadOnlyList<UsfmToken> tokens = tokenizer.Tokenize(usfm);
tokens = FilterTokensByChapter(tokens, chapters);

ScrVers parseVersification = _settings.Versification;
ScrVers rowsVersification = UpdateUsfmParserHandler.GetRowsVersification(rows);
if (rowsVersification != _settings.Versification)
{
    var converter = new ConvertUsfmVersificationHandler(rowsVersification);
    UsfmParser.Parse(tokens, converter, _settings.Stylesheet, _settings.Versification);
    tokens = converter.Tokens;
    parseVersification = rowsVersification;
}
UsfmParser.Parse(tokens, handler, _settings.Stylesheet, parseVersification);

You will need to add a Tokens property to the convert handler.

[Enkidu93]

OK, I've also added tests for invalid and missing verses/chapters to confirm that it won't crash the handler.

@Enkidu93 made 5 comments.
Reviewable status: all files reviewed (commit messages unreviewed), 4 unresolved discussions (waiting on ddaspit).

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 68 at r5 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

When we add a chapter marker in the middle of a paragraph, we should also add a paragraph marker. I think we want an \nb here (see new GetUsfm_SynthesizedChapter tests).

Done.

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 89 at r5 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

When we split a verse range across a chapter boundary, all of the verse's text ends up on the second half and the first half is left empty. I think we should put the text in the first verse (see new GetUsfm_SplitVerseRange_TextStaysWithFirstVerse test).

Done.

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 143 at r5 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

When a verse is dropped for mapping into another book, the headings that follow it get dropped too (see the two new GetUsfm_HeadingIntroducing tests).

Done.

---

src/SIL.Machine/Corpora/ParatextProjectVersificationConverterBase.cs line 9 at r5 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Rather than creating a new converter base class, could we integrate the conversion directly into ParatextProjectTextUpdaterBase? Something like this should work:

IReadOnlyList<UsfmToken> tokens = tokenizer.Tokenize(usfm);
tokens = FilterTokensByChapter(tokens, chapters);

ScrVers parseVersification = _settings.Versification;
ScrVers rowsVersification = UpdateUsfmParserHandler.GetRowsVersification(rows);
if (rowsVersification != _settings.Versification)
{
    var converter = new ConvertUsfmVersificationHandler(rowsVersification);
    UsfmParser.Parse(tokens, converter, _settings.Stylesheet, _settings.Versification);
    tokens = converter.Tokens;
    parseVersification = rowsVersification;
}
UsfmParser.Parse(tokens, handler, _settings.Stylesheet, parseVersification);

You will need to add a Tokens property to the convert handler.

OK, I've done as you said. I guess I was thinking there might be a use for this class outside of the updater, but I guess we can make a separate class as needed.

[ddaspit]

@ddaspit reviewed 5 files and all commit messages, made 7 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on Enkidu93).

---

src/SIL.Machine/Corpora/ParatextProjectVersificationConverterBase.cs line 9 at r5 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

OK, I've done as you said. I guess I was thinking there might be a use for this class outside of the updater, but I guess we can make a separate class as needed.

Did you mean to keep this class?

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 10 at r8 (raw file):

    public class ConvertUsfmVersificationHandler : ScriptureRefUsfmParserHandlerBase
    {
        private static readonly IReadOnlyList<Regex> TrailingParagraphMarkerPatterns = new List<Regex>

The regexes can potentially match too much. It would also be better to turn this into a single regex:

private static readonly Regex TrailingParagraphMarkerPattern =
    new Regex(@"^(?:mte?\d*|ms\d*|mr|s\d*|sr|r|d|sp|sd\d*)$", RegexOptions.Compiled);

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 141 at r8 (raw file):

                            {
                                _tokens.Add(nextToken);
                                _verseBoundary++;

It is possible for a verse range to increment _verseBoundary multiple times. You should ensure that it can only happen once for a verse marker.

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 105 at r9 (raw file):

                    _tokens.Add(newChapterToken);
                    _trailingVerseTokens = _trailingVerseTokens
                        .Select(tup => tup.Index == _tokens.Count - 1 ? (tup.Index + 1, tup.Token) : tup)

I think this should be tup.Index + 2.

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 230 at r9 (raw file):

                else if (inPreservedParagraph)
                {
                    inPreservedParagraph = !inPreservedParagraph || token.Type != UsfmTokenType.Paragraph;

I don't think the !inPreservedParagraph does anything.

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 242 at r9 (raw file):

                    _tokens.Add(token);
                else
                    offset = offset + 1 - 1;

I don't think this does anything.

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 117 at r8 (raw file):

        }

        public static ScrVers GetRowsVersification(IReadOnlyList<UpdateUsfmRow> rows)

Nit: the constructor duplicates this logic.

[Enkidu93]

@Enkidu93 made 7 comments and resolved 1 discussion.
Reviewable status: 5 of 8 files reviewed, 6 unresolved discussions (waiting on ddaspit).

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 10 at r8 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The regexes can potentially match too much. It would also be better to turn this into a single regex:

private static readonly Regex TrailingParagraphMarkerPattern =
    new Regex(@"^(?:mte?\d*|ms\d*|mr|s\d*|sr|r|d|sp|sd\d*)$", RegexOptions.Compiled);

Yes, silly me. Done. I did tweak s\d*...sd\d* to sd?\d*.

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 141 at r8 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

It is possible for a verse range to increment _verseBoundary multiple times. You should ensure that it can only happen once for a verse marker.

Done.

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 105 at r9 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I think this should be tup.Index + 2.

Why + 2? Because of the \nb? But this is added before _tokens adds the no-break. If you'd prefer, I could add the \nb first and then +2. Or maybe you're thinking something else I'm not picking up on?

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 230 at r9 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I don't think the !inPreservedParagraph does anything.

Right, yes. This was a bit in flux. Done.

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 242 at r9 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I don't think this does anything.

Yes, sorry - just no-op so I could put a breakpoint. Removed.

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 117 at r8 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Nit: the constructor duplicates this logic.

Done.

---

src/SIL.Machine/Corpora/ParatextProjectVersificationConverterBase.cs line 9 at r5 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Did you mean to keep this class?

Nope, done.

[ddaspit]

@ddaspit reviewed 4 files and all commit messages, made 2 comments, and resolved 6 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on Enkidu93).

---

src/SIL.Machine/Corpora/ConvertUsfmVersificationHandler.cs line 105 at r9 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Why + 2? Because of the \nb? But this is added before _tokens adds the no-break. If you'd prefer, I could add the \nb first and then +2. Or maybe you're thinking something else I'm not picking up on?

I looked into this a bit more and I feel like I'm tying myself into knots with all of the potential edge cases here. There are a couple of cases where we don't want to insert the \nb after the new chapter marker. I added a commit to fix it with some unit tests. It should take care of this.