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 89.70588% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.32%. Comparing base (ba0e245) to head (f349a58).

Files with missing lines	Patch %	Lines
src/SIL.Machine/Corpora/UsfmToken.cs	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #472      +/-   ##
==========================================
+ Coverage   73.30%   73.32%   +0.02%     
==========================================
  Files         445      445              
  Lines       37323    37380      +57     
  Branches     5120     5128       +8     
==========================================
+ Hits        27360    27410      +50     
- Misses       8836     8843       +7     
  Partials     1127     1127              

☔ 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).