Add versification warnings for invalid chapter or verse numbers in USFM #381

pmachapman · 2026-02-04T23:42:03Z

Fixes: #372

Example USFM:

\id MAN
\c 1.
\v v1 Verse 1

Example warnings:

warnings: [
  "USFM versification error in project TEA, expected verse “”, actual verse “MAN 1.”, mismatch type InvalidChapterNumber (parallel corpus 6983d7b5415e5f0ee3585264, monolingual corpus 6983d7b5415e5f0ee3585260)",
  "USFM versification error in project TEA, expected verse “”, actual verse “MAN -1:v1”, mismatch type InvalidVerseNumber (parallel corpus 6983d7b5415e5f0ee3585264, monolingual corpus 6983d7b5415e5f0ee3585260)",
]

This change is

codecov-commenter · 2026-02-04T23:42:53Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.79%. Comparing base (611fbf2) to head (5ca0162).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
+ Coverage   72.73%   72.79%   +0.06%     
==========================================
  Files         423      423              
  Lines       36029    36089      +60     
  Branches     4969     4974       +5     
==========================================
+ Hits        26205    26271      +66     
+ Misses       8733     8727       -6     
  Partials     1091     1091

☔ 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

The warning messages are a bit awkward. How difficult would it be to generate messages that are more appropriate for the new warnings?

@ddaspit reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93).

pmachapman

The warning messages are a bit awkward. How difficult would it be to generate messages that are more appropriate for the new warnings?

Good idea. That requires a modification to Serval - see sillsdev/serval/#871

@pmachapman made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93).

Enkidu93

@Enkidu93 reviewed 2 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @pmachapman).

src/SIL.Machine/Corpora/UsfmVersificationErrorDetector.cs line 49 at r1 (raw file):

        }

        public UsfmVersificationError(

Rather than adding another constructor, is there way to just check the error type in CheckError() itself?

src/SIL.Machine/Corpora/UsfmVersificationErrorDetector.cs line 94 at r1 (raw file):

                    return true;
                }
                if (!_verseRef.Value.Valid)

I'm surprised that these kinds of errors (especially the bad verse numbers) aren't captured in the verse ref's valid status somehow 🤔. It is what it is, but it'd certainly be nice if it were haha.

src/SIL.Machine/Corpora/UsfmVersificationErrorDetector.cs line 128 at r1 (raw file):

                    Type == UsfmVersificationErrorType.ExtraVerse
                    || Type == UsfmVersificationErrorType.InvalidChapterNumber
                    || Type == UsfmVersificationErrorType.InvalidVerseNumber

I wonder if we could supply an expected ref. We should have some idea of what verse or chapter is expected next while parsing. If we can make a reasonable guess, I think we should add it for consistency with our attempts for other types of errors, but if we can't no worries. If the bad chapter number is something like \c 1^, that's pretty easy to find without an expected ref, but what about something like \c text: Is that possible? That would not be quite so easy to track down.

src/SIL.Machine/Corpora/UsfmVersificationErrorDetector.cs line 292 at r1 (raw file):

            // See whether the chapter number is invalid
            VerseRef verseRef = state.VerseRef.Clone();

Why do we need to do this? Does the number get changed when the state is updated so we need the raw value itself? It would be nice if we could just pass the bad verse ref to the existing constructor and check for an error in CheckError() - same with invalid verses. If we have to do it, we have to do it, but it'd be nice to have the error checking all in one place.

src/SIL.Machine/Corpora/UsfmVersificationErrorDetector.cs line 332 at r1 (raw file):

                {
                    _errors.Add(versificationError);
                    verseInError = true;

What's the particular duplicate error situation you're trying to avoid?

pmachapman

@pmachapman made 5 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Enkidu93).

src/SIL.Machine/Corpora/UsfmVersificationErrorDetector.cs line 49 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Rather than adding another constructor, is there way to just check the error type in CheckError() itself?

I did that originally, but because we don't have an expected verse, and when I passed a verseRef it met the other conditions in CheckError(), it got confused with the other warnings. I figured another constructor made sense, as I also needed the actual string value to display to the user so they could resolve it. (If we don't show the actual string value to the user, the verse ref will only store "-1" for an invalid chapter, which isn't helpful to return in an error message)

src/SIL.Machine/Corpora/UsfmVersificationErrorDetector.cs line 94 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I'm surprised that these kinds of errors (especially the bad verse numbers) aren't captured in the verse ref's valid status somehow 🤔. It is what it is, but it'd certainly be nice if it were haha.

_verseRef.Value.Valid will return false, but as it is not clear just what the cause is (i.e. there is no comparable enum to UsfmVersificationErrorType returned by VerseRef), I deal with the invalid chapter/verse error just above this logic.

src/SIL.Machine/Corpora/UsfmVersificationErrorDetector.cs line 128 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I wonder if we could supply an expected ref. We should have some idea of what verse or chapter is expected next while parsing. If we can make a reasonable guess, I think we should add it for consistency with our attempts for other types of errors, but if we can't no worries. If the bad chapter number is something like \c 1^, that's pretty easy to find without an expected ref, but what about something like \c text: Is that possible? That would not be quite so easy to track down.

I tried that, but it is hard to tell what it should have been. Chapters can be skipped in a USFM file. If we were to parse the string and guess, then we might as well not throw an error and use the parsed string?

src/SIL.Machine/Corpora/UsfmVersificationErrorDetector.cs line 292 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Why do we need to do this? Does the number get changed when the state is updated so we need the raw value itself? It would be nice if we could just pass the bad verse ref to the existing constructor and check for an error in CheckError() - same with invalid verses. If we have to do it, we have to do it, but it'd be nice to have the error checking all in one place.

I get the raw value and pass it into the error object so we can return it in the error message, so that the error message doesn't just say something like "invalid chapter number -1" but will say the actual value that caused the error. (the VerseRef struct does not record what the invalid chapter number value is).

src/SIL.Machine/Corpora/UsfmVersificationErrorDetector.cs line 332 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

What's the particular duplicate error situation you're trying to avoid?

A missing verse error

ddaspit

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

Enkidu93

@Enkidu93 made 5 comments and resolved 4 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pmachapman).

src/SIL.Machine/Corpora/UsfmVersificationErrorDetector.cs line 49 at r1 (raw file):