Skip to content

Conversation

@jtmaxwell3
Copy link
Contributor

@jtmaxwell3 jtmaxwell3 commented Jun 6, 2025

This fixes https://jira.sil.org/browse/LT-22121. In order to handle cases where the occurrence was rAs but the wordform was ras, I changed the code to look up the wordform, the occurrence form, and the occurrence form with the first letter lowercased. These are usually all the same, but in rare cases they can all be different if the user chooses a wordform that isn't the occurrence form or an initial lowercase form of the occurrence form. This needed a lot of changes to the code. This should probably be saved for release/9.3.


This change is Reviewable

@github-actions
Copy link

github-actions bot commented Jun 6, 2025

LCM Tests

    16 files  ± 0      16 suites  ±0   2m 54s ⏱️ -15s
 2 834 tests + 3   2 814 ✅ + 3   20 💤 ±0  0 ❌ ±0 
11 284 runs  +12  11 116 ✅ +12  168 💤 ±0  0 ❌ ±0 

Results for commit d0ec124. ± Comparison against base commit 7fd440c.

♻️ This comment has been updated with latest results.

@jasonleenaylor
Copy link
Contributor

tests/SIL.LCModel.Tests/DomainServices/AnalysisGuessServicesTests.cs line 962 at r1 (raw file):

				// Don't guess lowercase even if it exists.
				IWfiAnalysis ras = WordAnalysisOrGlossServices.CreateNewAnalysisWAG(setup.Words_para0[21]);

For clarity I think you should move this below the first assert.

@jasonleenaylor
Copy link
Contributor

src/SIL.LCModel/DomainServices/AnalysisGuessServices.cs line 407 at r1 (raw file):

		}

		private bool HasContextCounts(IWfiWordform wordform)

I wouldn't expect a method named HasContextCounts to have significant side-affects, it seems like some clean-up to this section would be good.

Copy link
Contributor

@jasonleenaylor jasonleenaylor 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 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jtmaxwell3)

@jtmaxwell3 jtmaxwell3 merged commit 0eb28b5 into master Jun 16, 2025
5 checks passed
@jtmaxwell3 jtmaxwell3 deleted the LT-22121 branch June 16, 2025 16: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.

3 participants