Skip to content

Conversation

@ddaspit
Copy link
Contributor

@ddaspit ddaspit commented Sep 3, 2025


This change is Reviewable

@ddaspit ddaspit requested a review from Enkidu93 September 3, 2025 21:18
@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2025

Codecov Report

❌ Patch coverage is 77.77778% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.22%. Comparing base (9638a92) to head (6fc66e4).

Files with missing lines Patch % Lines
...chine/Corpora/ScriptureRefUsfmParserHandlerBase.cs 77.77% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #333   +/-   ##
=======================================
  Coverage   72.22%   72.22%           
=======================================
  Files         414      414           
  Lines       35154    35169   +15     
  Branches     4865     4871    +6     
=======================================
+ Hits        25389    25401   +12     
- Misses       8674     8676    +2     
- Partials     1091     1092    +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.

- properly handle verse 0
- ignore private-use markers
- fixes #329
Copy link
Collaborator

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Enkidu93 reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion


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

            if (state.VerseRef.Equals(_curVerseRef) && !_duplicateVerse)
            {
                if (state.VerseRef.VerseNum > 0)

Could we use state.IsVerseText instead?

Copy link
Contributor Author

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Enkidu93)


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

Previously, Enkidu93 (Eli C. Lowry) wrote…

Could we use state.IsVerseText instead?

This issue is specifically about the case where there is a \v 0 marker.

Copy link
Collaborator

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Do you want to put in a PR for the new Machine version or should I?

@Enkidu93 reviewed 1 of 2 files at r2.
Reviewable status: 1 of 2 files reviewed, all discussions resolved


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

Previously, ddaspit (Damien Daspit) wrote…

This issue is specifically about the case where there is a \v 0 marker.

OK, I was just thinking since the first thing IsVerseTextchecks is

if (VerseRef.VerseNum == 0)
    return false;
...

we could use it, but maybe that doesn't make as much sense semantically as I thought when I left the comment 😅.

Copy link
Collaborator

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 1 of 2 files reviewed, all discussions resolved

Copy link
Contributor Author

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ddaspit reviewed 1 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

@ddaspit ddaspit merged commit 196ca8e into master Sep 5, 2025
4 checks passed
@ddaspit ddaspit deleted the fix-usfm-crashes branch September 5, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

USFM Parsing Bug: 'Stack Empty'

4 participants