Port update block

[Enkidu93]

Port of changes here.

Fixes sillsdev/serval#667

---

This change is 

[Enkidu93]

One of the failing tests (UsfmMemoryTextTests.GetRows_OptBreak_OutsideOfSegment) looks like it has no analog in machine.py. Did that fix for ignoring opt breaks outside of segments not get ported to machine.py or has the expected behavior changed since then, @ddaspit?

[Enkidu93]

One of the failing tests (UsfmMemoryTextTests.GetRows_OptBreak_OutsideOfSegment) looks like it has no analog in machine.py. Did that fix for ignoring opt breaks outside of segments not get ported to machine.py or has the expected behavior changed since then, @ddaspit?

I see that it's connected to the change in IsVerseText. I'm going to assume that the behavior should be the same as it was before, fix it here, and port the test back.

[Enkidu93]

One of the failing tests (UsfmMemoryTextTests.GetRows_OptBreak_OutsideOfSegment) looks like it has no analog in machine.py. Did that fix for ignoring opt breaks outside of segments not get ported to machine.py or has the expected behavior changed since then, @ddaspit?

I see that it's connected to the change in IsVerseText. I'm going to assume that the behavior should be the same as it was before, fix it here, and port the test back.

After looking a bit more, I think that it might actually be behaving like it should be. I've pushed a change to the test to accommodate. Let me know if this doesn't look right, @ddaspit.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 93.50181% with 18 lines in your changes missing coverage. Please review.

Project coverage is 70.51%. Comparing base (fecb320) to head (f2f7fd7).

Files with missing lines	Patch %	Lines
src/SIL.Machine/Corpora/UsfmUpdateBlock.cs	78.18%	12 Missing ⚠️
src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs	97.05%	3 Missing and 2 partials ⚠️
...chine/Corpora/ScriptureRefUsfmParserHandlerBase.cs	96.87%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
- Coverage   70.53%   70.51%   -0.02%     
==========================================
  Files         386      388       +2     
  Lines       32311    32362      +51     
  Branches     4546     4545       -1     
==========================================
+ Hits        22791    22821      +30     
- Misses       8471     8491      +20     
- Partials     1049     1050       +1     

☔ View full report in Codecov by Sentry.
📢 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.

[ddaspit]

See my comment in UsfmMemoryTextTests.cs.

Reviewed 9 of 11 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Enkidu93)

---

src/SIL.Machine/Corpora/ScriptureRefUsfmParserHandlerBase.cs line 266 at r2 (raw file):

        }

        private void StartEmbedTextWrapper(UsfmParserState state, string marker)

C# supports method overloading, so this can be named StartEmbedText, like the other overloaded private methods in this class.

---

tests/SIL.Machine.Tests/Corpora/UsfmMemoryTextTests.cs line 219 at r3 (raw file):

\c 1
//
\p

The test does need to be fixed, but not in the way that you fixed it. The UsfmMemoryText class now correctly returns two rows. The first is an empty paragraph segment for text that can occur before the first verse marker. The second is the verse text.

---

src/SIL.Machine/Corpora/UsfmUpdateBlockHandler.cs line 3 at r2 (raw file):

namespace SIL.Machine.Corpora
{
    public abstract class UsfmUpdateBlockHandler

This should be an interface.

---

src/SIL.Machine/Corpora/UsfmUpdateBlock.cs line 8 at r2 (raw file):

    public class UsfmUpdateBlock
    {
        public List<ScriptureRef> Refs { get; }

These properties can be IReadOnlyLists. You will need to add backing fields for these properties.

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 408 at r2 (raw file):

                    (
                        CurrentTextType != ScriptureTextType.None
                        || (state.ParaTag != null && state.ParaTag.Marker == "id")

You can use the ?. operator to simplify this line.

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 447 at r2 (raw file):

                if (
                    CurrentTextType != ScriptureTextType.None
                    || (state.ParaTag != null && state.ParaTag.Marker == "id")

You can use the ?. operator to simplify this line.

[Enkidu93]

tests/SIL.Machine.Tests/Corpora/UsfmMemoryTextTests.cs line 219 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The test does need to be fixed, but not in the way that you fixed it. The UsfmMemoryText class now correctly returns two rows. The first is an empty paragraph segment for text that can occur before the first verse marker. The second is the verse text.

OK, yeah, I wasn't sure whether I should leave the \p and change the asserts or remove the \p and keep the asserts. Done here and done in machine.py PR.

[Enkidu93]

Reviewable status: 4 of 11 files reviewed, 6 unresolved discussions (waiting on @ddaspit)

---

src/SIL.Machine/Corpora/ScriptureRefUsfmParserHandlerBase.cs line 266 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

C# supports method overloading, so this can be named StartEmbedText, like the other overloaded private methods in this class.

Done

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 408 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You can use the ?. operator to simplify this line.

Done

---

src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 447 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You can use the ?. operator to simplify this line.

Done

---

src/SIL.Machine/Corpora/UsfmUpdateBlock.cs line 8 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

These properties can be IReadOnlyLists. You will need to add backing fields for these properties.

Done. Do you have a preference for the get => _field or get {return _field; } syntax. I used => since it looked cleaner, but I noticed that both are used frequently throughout the code-base.

---

src/SIL.Machine/Corpora/UsfmUpdateBlockHandler.cs line 3 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This should be an interface.

Done

[ddaspit]

Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

---

src/SIL.Machine/Corpora/UsfmUpdateBlock.cs line 8 at r2 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Done. Do you have a preference for the get => _field or get {return _field; } syntax. I used => since it looked cleaner, but I noticed that both are used frequently throughout the code-base.

The => syntax is cleaner.