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 86.46617% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.36%. Comparing base (ba0e245) to head (8012c89).
⚠️ Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
...rpora/ParatextProjectVersificationConverterBase.cs	69.69%	9 Missing and 1 partial ⚠️
...Machine/Corpora/ConvertUsfmVersificationHandler.cs	92.00%	5 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #472      +/-   ##
==========================================
+ Coverage   73.30%   73.36%   +0.05%     
==========================================
  Files         445      447       +2     
  Lines       37323    37456     +133     
  Branches     5120     5135      +15     
==========================================
+ Hits        27360    27479     +119     
- Misses       8836     8846      +10     
- Partials     1127     1131       +4     

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