Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix exception when parsing USFM with an empty verse paragraph #201

Merged
merged 5 commits into from
May 22, 2024

Conversation

ddaspit
Copy link
Contributor

@ddaspit ddaspit commented May 21, 2024

@ddaspit ddaspit force-pushed the empty-verse-para branch from 47663eb to 65140ba Compare May 21, 2024 21:15
@codecov-commenter
Copy link

codecov-commenter commented May 21, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 67.27%. Comparing base (61d792a) to head (7733533).

Files Patch % Lines
src/SIL.Machine/Corpora/ScriptureElement.cs 57.14% 2 Missing and 1 partial ⚠️
...chine/Corpora/ScriptureRefUsfmParserHandlerBase.cs 88.46% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
+ Coverage   67.24%   67.27%   +0.03%     
==========================================
  Files         441      441              
  Lines       34966    34993      +27     
  Branches     4689     4694       +5     
==========================================
+ Hits        23514    23543      +29     
+ Misses      10366    10361       -5     
- Partials     1086     1089       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@johnml1135
Copy link
Collaborator

@ddaspit - added more errors found from the sample project. You may have a specific way you want to fix them.

@johnml1135
Copy link
Collaborator

tests/SIL.Machine.Tests/Corpora/TestData/usfm/Tes/41MATTes.SFM line 5 at r2 (raw file):

\mt Matthew
\ip An introduction to Matthew\fe + \ft This is an endnote.\fe*
\p Here is another paragraph.

The current error is: System.InvalidOperationException : An error occurred while parsing the text 'MAT. Verse: MAT 1:0, offset: 179, error: 'Stack empty.'`. I still think that more context would be helpful - instead of people trying to count 179 tokens past the beginning of the verse (which starts exactly where?).

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.

Thanks for adding the errors. I will look into them.

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


tests/SIL.Machine.Tests/Corpora/TestData/usfm/Tes/41MATTes.SFM line 5 at r2 (raw file):

Previously, johnml1135 (John Lambert) wrote…

The current error is: System.InvalidOperationException : An error occurred while parsing the text 'MAT. Verse: MAT 1:0, offset: 179, error: 'Stack empty.'`. I still think that more context would be helpful - instead of people trying to count 179 tokens past the beginning of the verse (which starts exactly where?).

We could do line number and column instead of verse ref and verse offset. It would be a bit more work, since the UsfmTokenizer and UsfmParser aren't tracking line number and column, but it should be doable.

@johnml1135
Copy link
Collaborator

tests/SIL.Machine.Tests/Corpora/TestData/usfm/Tes/41MATTes.SFM line 5 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

We could do line number and column instead of verse ref and verse offset. It would be a bit more work, since the UsfmTokenizer and UsfmParser aren't tracking line number and column, but it should be doable.

Line number and offset (along with verse ref) would be nice - let's do it.

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

@johnml1135
Copy link
Collaborator

johnml1135 commented May 22, 2024

@ddaspit - Failed GetUsfm_NonVerse_Paragraph [84 ms]

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

@johnml1135 johnml1135 merged commit 8d84ddd into master May 22, 2024
4 checks passed
@johnml1135 johnml1135 deleted the empty-verse-para branch May 22, 2024 21:58
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.

New Empty Stack USFM bombing issue
3 participants