Skip to content

Conversation

@pmachapman
Copy link
Collaborator

@pmachapman pmachapman commented Oct 6, 2025

This PR updates the Serval Client to version 1.12 (the version currently on QA).

This update to Serval.Client marks several the older corpus API endpoints as deprecated, so the code now clearly needs to demarcate whether a parallel corpus or a legacy corpus is being used to retrieve pre-translations.

This PR should only be merged when Serval 1.12 is deployed to production (likely later this week).

The current version of Serval.Client (1.10) will continue to work as per normal when Serval production is updated, so this PR is not a blocker in any way.


This change is Reviewable

@codecov
Copy link

codecov bot commented Oct 6, 2025

Codecov Report

❌ Patch coverage is 94.93671% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.27%. Comparing base (de136f1) to head (d40c2ad).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...XForge.Scripture/Services/PreTranslationService.cs 94.93% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3495      +/-   ##
==========================================
+ Coverage   82.25%   82.27%   +0.02%     
==========================================
  Files         615      615              
  Lines       37142    37187      +45     
  Branches     6071     6055      -16     
==========================================
+ Hits        30550    30595      +45     
- Misses       5688     5701      +13     
+ Partials      904      891      -13     

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

Copy link
Collaborator Author

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

@pmachapman dismissed @github-advanced-security[bot] from 6 discussions.
Reviewable status: 0 of 8 files reviewed, all discussions resolved

@Nateowami Nateowami added the e2e Run e2e tests for this pull request label Oct 6, 2025
Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

@Nateowami reviewed 6 of 8 files at r1, all commit messages.
Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion


src/SIL.XForge.Scripture/Services/PreTranslationService.cs line 217 at r1 (raw file):

                textOrigin: PretranslationUsfmTextOrigin.OnlyPretranslated,
                template: PretranslationUsfmTemplate.Source,
                paragraphMarkerBehavior: config.ParagraphFormat switch

Could we move these two switch expressions to before the if statement, so they're precomputed, and don't have to be manually kept in sync with each other, and reviewed to make sure they're the same? (in other words, calculate the paragraphMarkerBehavior and quoteNormalizationBehavior once rather than twice).

@pmachapman pmachapman removed the e2e Run e2e tests for this pull request label Oct 8, 2025
@pmachapman pmachapman force-pushed the update/serval-client branch from 5714bd5 to 90c90f3 Compare October 8, 2025 18:41
Copy link
Collaborator Author

@pmachapman pmachapman 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: 6 of 8 files reviewed, all discussions resolved (waiting on @Nateowami)


src/SIL.XForge.Scripture/Services/PreTranslationService.cs line 217 at r1 (raw file):

Previously, Nateowami wrote…

Could we move these two switch expressions to before the if statement, so they're precomputed, and don't have to be manually kept in sync with each other, and reviewed to make sure they're the same? (in other words, calculate the paragraphMarkerBehavior and quoteNormalizationBehavior once rather than twice).

Done. That looks better - thanks!

@pmachapman pmachapman force-pushed the update/serval-client branch from 90c90f3 to aa22af2 Compare October 19, 2025 18:42
@pmachapman pmachapman added the do not merge See PR description and/or comments for explanation label Oct 22, 2025
@pmachapman pmachapman force-pushed the update/serval-client branch from aa22af2 to db221a9 Compare October 27, 2025 19:11
Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

@Nateowami reviewed 5 of 5 files at r3.
Reviewable status: 8 of 10 files reviewed, 3 unresolved discussions


src/SIL.XForge.Scripture/Services/PreTranslationService.cs line 217 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Done. That looks better - thanks!

(Sorry for the slow review. I paused on this section yesterday morning when I realized the logic didn't look right and never got back to it).


src/SIL.Converters.Usj/packages.lock.json line 27 at r3 (raw file):

    }
  }
}

It looks like the only change to this file is the removal of a new line.


src/SIL.XForge/packages.lock.json line 1186 at r3 (raw file):

    }
  }
}

It looks like the only change to this file is the removal of a new line.


src/SIL.XForge.Scripture/packages.lock.json line 2202 at r3 (raw file):

    }
  }
}

It looks like the new line at the end of the file was removed.

@pmachapman pmachapman force-pushed the update/serval-client branch from db221a9 to d40c2ad Compare October 29, 2025 00:49
Copy link
Collaborator Author

@pmachapman pmachapman 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: 8 of 10 files reviewed, 3 unresolved discussions (waiting on @Nateowami)


src/SIL.Converters.Usj/packages.lock.json line 27 at r3 (raw file):

Previously, Nateowami wrote…

It looks like the only change to this file is the removal of a new line.

Done - reverted - thank you!


src/SIL.XForge/packages.lock.json line 1186 at r3 (raw file):

Previously, Nateowami wrote…

It looks like the only change to this file is the removal of a new line.

Done - reverted - thank you!


src/SIL.XForge.Scripture/packages.lock.json line 2202 at r3 (raw file):

Previously, Nateowami wrote…

It looks like the new line at the end of the file was removed.

Done - reverted - thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge See PR description and/or comments for explanation testing not required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants