Repository navigation
LT-22780: Improve Hermit Crab performance - #1176
Conversation
NUnit Tests 1 files ± 0 1 suites ±0 13m 30s ⏱️ -3s Results for commit 092b8fa. ± Comparison against base commit a6d34bd. This pull request removes 6 and adds 224 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1176 +/- ##
==========================================
+ Coverage 39.30% 39.51% +0.20%
==========================================
Files 1523 1537 +14
Lines 353117 354706 +1589
Branches 40761 41096 +335
==========================================
+ Hits 138806 140159 +1353
- Misses 185038 185154 +116
- Partials 29273 29393 +120
🚀 New features to boost your workflow:
|
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
LGTM, one question inline.
This review was assisted by Claude Opus 5.5.
| stopWatch.Stop(); | ||
| lcResult.ParseTime = stopWatch.ElapsedMilliseconds; | ||
| if (lcResult.Analyses.Count > 0 && lcResult.ErrorMessage == null) | ||
| if (lcResult.Analyses.Count > 0 || !String.IsNullOrEmpty(lcResult.ErrorMessage)) |
There was a problem hiding this comment.
Filing a lowercase result that has only an error message now creates a lowercase wordform: ParseFiler.UpdateWordforms calls FindOrCreateWordform for it. Since this path runs for ordinary background parsing too, not just Check Parser, a capitalized word whose lowercase parse errors leaves a new lowercase wordform in the project holding only that error. Was that intended? If the error is only wanted for the parser report, checking checkParser here would keep it out of the project.
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
@jasonleenaylor reviewed 10 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on jtmaxwell3).
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
Thanks, that addresses it. LGTM.
This review was assisted by Claude Opus 5.5.
| stopWatch.Stop(); | ||
| lcResult.ParseTime = stopWatch.ElapsedMilliseconds; | ||
| if (lcResult.Analyses.Count > 0 && lcResult.ErrorMessage == null) | ||
| if (lcResult.Analyses.Count > 0 || (checkParser && !String.IsNullOrEmpty(lcResult.ErrorMessage))) |
There was a problem hiding this comment.
Under Check Parser, an error-only lowercase result still looks like it creates the lowercase wordform, because ParseFiler.UpdateWordforms calls FindOrCreateWordform before it checks CheckParserUpdatesAnalyses. So even a Check Parser run that is "just a test" can add one. I'm not sure if this is an issue for real world use or not so I'm just pointing it out for you to consider.
3 participants
This implements https://jira.sil.org/browse/LT-22780. It includes three performance enhancements and some bug fixes that I found when writing the acceptance tests:
This change is