Skip to content

Conversation

@jtmaxwell3
Copy link
Contributor

@jtmaxwell3 jtmaxwell3 commented Jul 11, 2025

liblcm support for https://jira.sil.org/browse/LT-21902. I make IMoInflAffixSlot cloneable and I make both slots and templates have unique names when they get cloned.


This change is Reviewable

@github-actions
Copy link

github-actions bot commented Jul 11, 2025

LCM Tests

    16 files  ±0      16 suites  ±0   3m 14s ⏱️ +16s
 2 835 tests ±0   2 815 ✅ ±0   20 💤 ±0  0 ❌ ±0 
11 288 runs  ±0  11 120 ✅ ±0  168 💤 ±0  0 ❌ ±0 

Results for commit 81381f5. ± Comparison against base commit 9588c57.

♻️ This comment has been updated with latest results.

@jasonleenaylor
Copy link
Contributor

src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs line 2925 at r2 (raw file):

			foreach (IMoInflAffMsa msa in Affixes)
			{
				if (msa.Owner is not ILexEntry entry)

This could use a comment. If there is an item in the affixes which is not an ILexEntry it won't be part of the clone and the reason should be given.

@jtmaxwell3
Copy link
Contributor Author

If it is not a LexEntry, then the next line would crash. I don't think that it can be anything but a LexEntry, but I'm trying to be more defensive in my programming.

@jasonleenaylor
Copy link
Contributor

src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs line 2925 at r2 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

This could use a comment. If there is an item in the affixes which is not an ILexEntry it won't be part of the clone and the reason should be given.

In that case just a comment explaining that this is never expected but we'd rather not crash is good enough.
Just to keep the next developer from wondering.

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.

:lgtm:

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

@jtmaxwell3 jtmaxwell3 merged commit 76fad9c into master Jul 15, 2025
5 checks passed
@jtmaxwell3 jtmaxwell3 deleted the LT-21902 branch July 15, 2025 16:45
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