Fix unclosed style marker crash

[benjaminking]

This PR corrects a few different errors that could occur with marker placement, due to unmatched markers in the source, one of which was causing a crash, and the others leading to incorrect output:

• An unclosed style marker in a non-verse paragraph would cause a crash
• An unclosed style marker would cause the next occurring paragraph marker to be ignored
• Only the first row of a verse range was being read
• An element of type OTHER could cause markers to be placed out of order
• Plus one corner case where end_note was behaving inconsistently in duplicated verses

---

This change is 

[Enkidu93]

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

---

tests/corpora/test_place_markers_usfm_update_block_handler.py line 732 at r1 (raw file):

def test_unclosed_style_marker_in_non_verse_paragraph() -> None:

I'm assuming that all these tests were previously failing since you've added them or are covering a portion of uncovered code? I'm just surprised that all of these would have failed.

---

tests/corpora/test_place_markers_usfm_update_block_handler.py line 873 at r1 (raw file):

def test_verse_range_matched_by_multiple_rows() -> None:
    # A verse range picks up one text token per matched row, so all of them have to be read.

I'm unclear on what the point of this test is exactly. The combination of the test name and the comment aren't making sense to me. Is this really just a test that verse ranges are handled properly? Or is the point that there's duplicate text?

---

tests/corpora/test_place_markers_usfm_update_block_handler.py line 804 at r1 (raw file):

def test_marker_behavior_disagrees_with_alignment_info() -> None:
    # The behaviors in the alignment info are supplied by the caller and can disagree with the

Are both necessary? Having looked into this while making this fix, do recall if these is a clear reason we would need the behavior specified in both places?

[Enkidu93]

Thank you for digging into this, Ben!

@Enkidu93 made 1 comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on benjaminking and ddaspit).

[benjaminking]

@benjaminking reviewed 1 file and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on ddaspit and Enkidu93).

---

tests/corpora/test_place_markers_usfm_update_block_handler.py line 732 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I'm assuming that all these tests were previously failing since you've added them or are covering a portion of uncovered code? I'm just surprised that all of these would have failed.

I don't think the marker placement code was ever tested for unmatched markers. I do believe all of these tests are covering behavior that was not tested previously (and I'm pretty sure these were all failing tests)

---

tests/corpora/test_place_markers_usfm_update_block_handler.py line 804 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Are both necessary? Having looked into this while making this fix, do recall if these is a clear reason we would need the behavior specified in both places?

I looked into it, and the behavior between the two is a little bit different, so we can't just use one or the other. I do, however, think this design is unintuitive, but I'll try to address that in a separate PR.

---

tests/corpora/test_place_markers_usfm_update_block_handler.py line 873 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I'm unclear on what the point of this test is exactly. The combination of the test name and the comment aren't making sense to me. Is this really just a test that verse ranges are handled properly? Or is the point that there's duplicate text?

The marker placement algorithm was only pulling out the first text row when it was reconstructing a verse range. It was essentially hard-coded for single verses.

[Enkidu93]

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

---

tests/corpora/test_place_markers_usfm_update_block_handler.py line 732 at r1 (raw file):

Previously, benjaminking (Ben King) wrote…

I don't think the marker placement code was ever tested for unmatched markers. I do believe all of these tests are covering behavior that was not tested previously (and I'm pretty sure these were all failing tests)

OK, great! Thanks for catching this. It's crazy how many tests there are and yet still things like this could be broken.

---

tests/corpora/test_place_markers_usfm_update_block_handler.py line 804 at r1 (raw file):

Previously, benjaminking (Ben King) wrote…

I looked into it, and the behavior between the two is a little bit different, so we can't just use one or the other. I do, however, think this design is unintuitive, but I'll try to address that in a separate PR.

OK, feel free to just create an issue as well to track it as future works with your conclusions - I don't want to keep you from other things.

---

tests/corpora/test_place_markers_usfm_update_block_handler.py line 873 at r1 (raw file):

Previously, benjaminking (Ben King) wrote…

The marker placement algorithm was only pulling out the first text row when it was reconstructing a verse range. It was essentially hard-coded for single verses.

I see, OK. I'm still not 100% following the comment or the test name. The 'verse range is matched by multiple rows'? The 'verse range picks up a text token per matched row'? Maybe I'm not reading them correctly but these don't make sense to me. If the important part is just that it's testing a verse range, we could probably just keep it at that. If it's important that the same tokens occur across parts of the range, then maybe mention that in the comment? Or if you feel that these are already clear and relevant, could you explain what they mean to me? 😆

[benjaminking]

@benjaminking made 1 comment.
Reviewable status: 7 of 8 files reviewed, 3 unresolved discussions (waiting on ddaspit and Enkidu93).

---

tests/corpora/test_place_markers_usfm_update_block_handler.py line 873 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I see, OK. I'm still not 100% following the comment or the test name. The 'verse range is matched by multiple rows'? The 'verse range picks up a text token per matched row'? Maybe I'm not reading them correctly but these don't make sense to me. If the important part is just that it's testing a verse range, we could probably just keep it at that. If it's important that the same tokens occur across parts of the range, then maybe mention that in the comment? Or if you feel that these are already clear and relevant, could you explain what they mean to me? 😆

I have updated the name of the test as well as the comments. It was definitely a bit of the Claude code idiolect.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 98.94737% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.12%. Comparing base (b09498c) to head (d1932d7).

Files with missing lines	Patch %	Lines
...corpora/place_markers_usfm_update_block_handler.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #364      +/-   ##
==========================================
+ Coverage   92.10%   92.12%   +0.02%     
==========================================
  Files         389      390       +1     
  Lines       24559    24635      +76     
==========================================
+ Hits        22620    22695      +75     
- Misses       1939     1940       +1     

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

@Enkidu93 reviewed all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on benjaminking and ddaspit).

---

tests/corpora/test_place_markers_usfm_update_block_handler.py line 873 at r1 (raw file):

Previously, benjaminking (Ben King) wrote…

I have updated the name of the test as well as the comments. It was definitely a bit of the Claude code idiolect.

Thank you - much better 😁