Skip to content

Updates to USFM updater#291

Merged
johnml1135 merged 4 commits intomasterfrom
bov_embeds
Mar 10, 2025
Merged

Updates to USFM updater#291
johnml1135 merged 4 commits intomasterfrom
bov_embeds

Conversation

@johnml1135
Copy link
Copy Markdown
Collaborator

@johnml1135 johnml1135 commented Mar 7, 2025

Add tests for beginning-of-verse embeds
Fix the embed at beginning issue (sillsdev/serval#636)
Add paragraph marker control (sillsdev/serval#627)
Always preserve \r and \rem
Correct behavior for stripping text behavior (sillsdev/serval#629)


This change is Reviewable

@johnml1135 johnml1135 requested a review from ddaspit March 7, 2025 15:56
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 7, 2025

Codecov Report

❌ Patch coverage is 95.71429% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.46%. Comparing base (d530590) to head (970f84d).
⚠️ Report is 82 commits behind head on master.

Files with missing lines Patch % Lines
src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs 95.45% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
+ Coverage   70.42%   70.46%   +0.03%     
==========================================
  Files         386      386              
  Lines       32204    32256      +52     
  Branches     4531     4539       +8     
==========================================
+ Hits        22680    22729      +49     
- Misses       8478     8479       +1     
- Partials     1046     1048       +2     

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

@johnml1135 johnml1135 changed the title Add tests for beginning-of-verse embeds Updates to USFM updater Mar 7, 2025
Copy link
Copy Markdown
Contributor

@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.

:lgtm:

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)

Fix the embed at beginning issue.
Add paragraph marker control
Correct behavior for stripping text
* If there is new text, override "preserve"
* Preserve just means "don't strip" this tag
* Make "preserve" configurable and at the "update" level, not the "scripture" level
* Correct logic only stripping paragraphs in a verse - not section headers
Copy link
Copy Markdown
Contributor

@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.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)


src/SIL.Machine/Corpora/ParatextProjectTextUpdaterBase.cs line 31 at r2 (raw file):

            UpdateUsfmMarkerBehavior embedBehavior = UpdateUsfmMarkerBehavior.Preserve,
            UpdateUsfmMarkerBehavior styleBehavior = UpdateUsfmMarkerBehavior.Strip,
            ImmutableHashSet<string> preserveParagraphStyles = null

It would be better to pass in an interface, i.e. IReadOnlySet, so that the caller has more flexibility.

@johnml1135
Copy link
Copy Markdown
Collaborator Author

src/SIL.Machine/Corpora/ParatextProjectTextUpdaterBase.cs line 31 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

It would be better to pass in an interface, i.e. IReadOnlySet, so that the caller has more flexibility.

Done.

Copy link
Copy Markdown
Contributor

@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.

Reviewed 1 of 3 files at r3, all commit messages.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @johnml1135)


src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs line 36 at r3 (raw file):

        private readonly UpdateUsfmMarkerBehavior _embedBehavior;
        private readonly UpdateUsfmMarkerBehavior _styleBehavior;
        private readonly IReadOnlySet<string> _preserveParagraphStyles;

If you make this HashSet, then there is no need to cast when you assign the field. Also, I forgot that .NET Standard 2.0 doesn't support IReadOnlySet.

Copy link
Copy Markdown
Contributor

@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.

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

@johnml1135 johnml1135 merged commit 806e171 into master Mar 10, 2025
4 checks passed
@johnml1135 johnml1135 deleted the bov_embeds branch March 10, 2025 20:48
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.

3 participants