Skip to content

Conversation

@Enkidu93
Copy link
Collaborator

@Enkidu93 Enkidu93 commented Dec 16, 2025

Partial fix of #844. Requires sillsdev/machine#368. Fixes #841.

Includes:

  • Cap the number of warnings to 100. (Is that a good number? And do we want that to be configurable through options?)
  • Utilize changes in machine to expose project name
  • Reformat build warning to put information most relevant to EITL team at the beginning + improve readability of verse references by wrapping in directional quotes
  • Only analyze each file once to avoid duplicate errors

This change is Reviewable

@Enkidu93 Enkidu93 requested a review from ddaspit December 16, 2025 21:32
Copy link
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.

@ddaspit reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)


src/Machine/src/Serval.Machine.Shared/Services/NmtPreprocessBuildJob.cs line 90 at r1 (raw file):

        );

        int maxWarnings = 100;

Yes, let's make this configurable. One hundred seems a bit low. Maybe 1000.


src/ServiceToolkit/src/SIL.ServiceToolkit/Services/IParallelCorpusPreprocessingService.cs line 7 at r1 (raw file):

    QuoteConventionAnalysis? AnalyzeTargetCorpusQuoteConvention(ParallelCorpus corpus);
    IReadOnlyList<(string CorpusId, IReadOnlyList<UsfmVersificationError> Errors)> AnalyzeUsfmVersification(
        ParallelCorpus parallelCorpus

I appreciate these small changes that incrementally improve the quality of the codebase.

Copy link
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:

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

author Enkidu93 <[email protected]> 1765920252 -0500
committer Enkidu93 <[email protected]> 1766155017 -0500

Cap the number of warnings to 100; add project name to warning and reformat warning message for better readability; Only generate warnings for each file once

Add max warnings to build options; fix filtering logic; add tests

Fix version typo
@Enkidu93 Enkidu93 force-pushed the improve_usfm_versification_warnings branch from 6416a7e to 5a61c4d Compare December 19, 2025 14:37
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.18%. Comparing base (3e15d8f) to head (5a61c4d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #847      +/-   ##
==========================================
+ Coverage   66.11%   66.18%   +0.07%     
==========================================
  Files         382      382              
  Lines       20744    20789      +45     
  Branches     2711     2719       +8     
==========================================
+ Hits        13714    13759      +45     
  Misses       6066     6066              
  Partials      964      964              

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Enkidu93 Enkidu93 merged commit 61a381b into main Dec 19, 2025
2 of 3 checks passed
@Enkidu93 Enkidu93 deleted the improve_usfm_versification_warnings branch December 19, 2025 15:53
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.

Build fails when sending message with a large number of versification warnings

4 participants