Add morph types to MiniLcm #1857
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change introduces the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
rmunn
changed the title
C# Unit Tests130 tests 130 ✅ 19s ⏱️ Results for commit fc34c25. ♻️ This comment has been updated with latest results. |
|
I think that's everything. @hahn-kev - Want to bikeshed my choice of MorphType for the enum and MorphTypeData for the class with the name, abbreviation, etc? I'd like to nail down the naming before I go much farther, or there will be a lot of code to rename. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs (1)
42-46: Consider consistent behavior for getter and setter.The MorphType property has inconsistent behavior - the getter throws
NotImplementedExceptionwhile the setter only logs a message. For better consistency and clearer intent, consider either implementing both operations or having both throwNotImplementedException.If this is temporary (as the comment suggests), consider using the same approach for both:
public override MorphType MorphType { - get => throw new NotImplementedException(); - set => Console.WriteLine("setting MorphType not implemented"); // Not throwing, for now + get => throw new NotImplementedException("MorphType getter not implemented"); + set => throw new NotImplementedException("MorphType setter not implemented"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs(3 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs(2 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs(2 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateMorphTypeDataProxy.cs(1 hunks)backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs(1 hunks)backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs(1 hunks)backend/FwLite/MiniLcm/IMiniLcmReadApi.cs(1 hunks)backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs(1 hunks)backend/FwLite/MiniLcm/Models/Entry.cs(2 hunks)backend/FwLite/MiniLcm/Models/MorphType.cs(1 hunks)backend/FwLite/MiniLcm/SyncHelpers/IntegerDiff.cs(1 hunks)backend/FwLite/MiniLcm/SyncHelpers/MorphTypeDataSync.cs(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IEntry.ts(2 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/MorphType.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (2)
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.386Z
Learning: In the sillsdev/languageforge-lexbox project, when file size limits or other constants need to be shared between C# backend and TypeScript frontend code, prefer exposing them through Reinforced.Typings type generation rather than hardcoding the values separately. This ensures consistency and prevents discrepancies when values change.
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:20:03.040Z
Learning: The user wants to share the 10MB file size limit constant between C# and TypeScript using Reinforced.Typings to ensure consistency and prevent hardcoding discrepancies like the current typo (1034 vs 1024).
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IEntry.ts (4)
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.386Z
Learning: In the sillsdev/languageforge-lexbox project, when file size limits or other constants need to be shared between C# backend and TypeScript frontend code, prefer exposing them through Reinforced.Typings type generation rather than hardcoding the values separately. This ensures consistency and prevents discrepancies when values change.
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:20:03.040Z
Learning: The user wants to share the 10MB file size limit constant between C# and TypeScript using Reinforced.Typings to ensure consistency and prevent hardcoding discrepancies like the current typo (1034 vs 1024).
Learnt from: myieye
PR: #1536
File: backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs:43-44
Timestamp: 2025-03-17T13:16:24.769Z
Learning: In the SIL FieldWorks Language Explorer (FLEx) system, only certain fields support rich text formatting (embedded styles): Definition, Example sentences, Translations, Notes, and Literal meaning fields. Other fields like Gloss, Lexeme form, and Citation form are plain text only. The codebase reflects this distinction by using RichMultiString for rich-text capable fields and MultiString for plain text fields.
Learnt from: myieye
PR: #1536
File: backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs:43-44
Timestamp: 2025-03-17T13:16:24.769Z
Learning: In the SIL FieldWorks Language Explorer (FLEx) system, only certain fields support rich text formatting (embedded styles): Definition, Example sentence, Translation, Note, and Literal meaning fields. Other fields like Gloss, Lexeme form, and Citation form are plain text only. The codebase reflects this by using RichMultiString for rich-text capable fields and MultiString for plain text fields.
backend/FwLite/MiniLcm/IMiniLcmReadApi.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/MorphType.ts (2)
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.386Z
Learning: In the sillsdev/languageforge-lexbox project, when file size limits or other constants need to be shared between C# backend and TypeScript frontend code, prefer exposing them through Reinforced.Typings type generation rather than hardcoding the values separately. This ensures consistency and prevents discrepancies when values change.
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:20:03.040Z
Learning: The user wants to share the 10MB file size limit constant between C# and TypeScript using Reinforced.Typings to ensure consistency and prevent hardcoding discrepancies like the current typo (1034 vs 1024).
backend/FwLite/MiniLcm/Models/Entry.cs (2)
Learnt from: myieye
PR: #1536
File: backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs:43-44
Timestamp: 2025-03-17T13:16:24.769Z
Learning: In the SIL FieldWorks Language Explorer (FLEx) system, only certain fields support rich text formatting (embedded styles): Definition, Example sentences, Translations, Notes, and Literal meaning fields. Other fields like Gloss, Lexeme form, and Citation form are plain text only. The codebase reflects this distinction by using RichMultiString for rich-text capable fields and MultiString for plain text fields.
Learnt from: myieye
PR: #1536
File: backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs:43-44
Timestamp: 2025-03-17T13:16:24.769Z
Learning: In the SIL FieldWorks Language Explorer (FLEx) system, only certain fields support rich text formatting (embedded styles): Definition, Example sentence, Translation, Note, and Literal meaning fields. Other fields like Gloss, Lexeme form, and Citation form are plain text only. The codebase reflects this by using RichMultiString for rich-text capable fields and MultiString for plain text fields.
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs (1)
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.386Z
Learning: In the sillsdev/languageforge-lexbox project, when file size limits or other constants need to be shared between C# backend and TypeScript frontend code, prefer exposing them through Reinforced.Typings type generation rather than hardcoding the values separately. This ensures consistency and prevents discrepancies when values change.
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateMorphTypeDataProxy.cs (1)
Learnt from: hahn-kev
PR: #1836
File: backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/MediaTests.cs:28-33
Timestamp: 2025-07-29T07:10:53.376Z
Learning: In test code for FwData-specific functionality (like MediaTests in FwDataMiniLcmBridge.Tests), direct casting to FwDataMiniLcmApi is preferred over safe casting because it serves as an assertion that the test setup is correct. If the cast fails, it indicates a test configuration bug that should be caught immediately rather than silently ignored.
backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (2)
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.386Z
Learning: In the sillsdev/languageforge-lexbox project, when file size limits or other constants need to be shared between C# backend and TypeScript frontend code, prefer exposing them through Reinforced.Typings type generation rather than hardcoding the values separately. This ensures consistency and prevents discrepancies when values change.
Learnt from: rmunn
PR: #1795
File: backend/FwLite/MiniLcm/Validators/PublicationValidator.cs:15-23
Timestamp: 2025-07-03T09:52:39.059Z
Learning: In the sillsdev/languageforge-lexbox project, MultiString objects have a custom .ToString() method that joins all strings in the MultiString with ", ". When generating identifiers for publications, the code intentionally uses MultiString.ToString() to show all names from different writing systems, not just the first one.
backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
backend/FwLite/MiniLcm/Models/MorphType.cs (2)
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.386Z
Learning: In the sillsdev/languageforge-lexbox project, when file size limits or other constants need to be shared between C# backend and TypeScript frontend code, prefer exposing them through Reinforced.Typings type generation rather than hardcoding the values separately. This ensures consistency and prevents discrepancies when values change.
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
backend/FwLite/MiniLcm/SyncHelpers/MorphTypeDataSync.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (2)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (2)
Learnt from: hahn-kev
PR: #1836
File: backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/MediaTests.cs:28-33
Timestamp: 2025-07-29T07:10:53.376Z
Learning: In test code for FwData-specific functionality (like MediaTests in FwDataMiniLcmBridge.Tests), direct casting to FwDataMiniLcmApi is preferred over safe casting because it serves as an assertion that the test setup is correct. If the cast fails, it indicates a test configuration bug that should be caught immediately rather than silently ignored.
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
🔇 Additional comments (28)
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (1)
133-133: LGTM! Proper enum export configuration.The addition follows the established pattern for exporting enums as string types, ensuring consistent type safety between the C# backend and TypeScript frontend for the new MorphType functionality.
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IEntry.ts (2)
9-9: LGTM! Correct import for MorphType.The import statement properly adds the MorphType type dependency for the new morphType property.
22-22: LGTM! Property addition aligns with backend changes.The new morphType property correctly reflects the MorphType property added to the Entry model in the backend, maintaining type consistency across the stack.
backend/FwLite/MiniLcm/Models/Entry.cs (2)
53-53: LGTM! Copy method properly updated.The Copy method correctly includes the new MorphType property, maintaining the integrity of the copy operation.
13-13: Enum default verified:Unknown = 0The
MorphTypeenum inbackend/FwLite/MiniLcm/Models/MorphType.cscorrectly declaresUnknownas its first member, so any uninitializedMorphTypeproperty will default toUnknown. No changes are needed.• File: backend/FwLite/MiniLcm/Models/MorphType.cs
Lines 6–16:public enum MorphType { Unknown, BoundRoot, BoundStem, Circumfix, Clitic, Enclitic, Infix, Particle, Prefix, }backend/FwLite/MiniLcm/IMiniLcmReadApi.cs (2)
15-15: LGTM! Method signature follows established patterns.The GetAllMorphTypeData method correctly uses IAsyncEnumerable for efficient streaming, consistent with other collection retrieval methods in the interface.
17-17: LGTM! Proper single-item retrieval method.The GetMorphTypeData method follows the established pattern for single-item retrieval with nullable return type and Guid parameter, maintaining consistency with other similar methods.
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/MorphType.ts (1)
6-28: ✅ MorphType enum matches C# definitionI’ve compared the generated TypeScript enum in
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/MorphType.ts
against the backend C# enum in
backend/FwLite/MiniLcm/Models/MorphType.cs. They are identical and, since this file is auto-generated via Reinforced.Typings, it will stay in sync with the source. No further action required.backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs (1)
38-50: LGTM! Consistent API design with good organization.The new MorphType CRUD operations follow the established patterns used for other entities in the API. The addition of region blocks for both ComplexFormType and MorphType improves code organization and readability.
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs (1)
24-24: LGTM! Proper integration of morph type awareness.The lexeme form creation now correctly uses the morph type information from the LCM entry, which aligns with the new morph type support throughout the system.
backend/FwLite/MiniLcm/Models/MorphType.cs (3)
5-29: LGTM! Comprehensive morph type enumeration.The enum provides a thorough set of morphological categories with proper JSON serialization support. The variety of morph types covers standard linguistic analysis needs well.
53-67: LGTM! Proper deep copy implementation.The
Copy()method correctly performs deep copies of all properties, including the multilingual strings and preserving the deletion timestamp.
44-51: No action required: MorphType has no entity referencesThe empty implementations of GetReferences() and RemoveReference() in MorphType are intentional—this model doesn’t reference any other entities. This is consistent with other “leaf” models (e.g., Publication, WritingSystem, SemanticDomain, Entry, ComplexFormType) that likewise return an empty array and no-op RemoveReference.
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateMorphTypeDataProxy.cs (1)
6-53: LGTM! Well-implemented proxy following established patterns.The proxy correctly wraps
IMoMorphTypeand maps its properties to theMorphTypeDatainterface. The property mappings are logical (e.g.,Prefix→LeadingToken,Postfix→TrailingToken), and the implementation follows the same pattern as other UpdateProxy classes in the codebase.backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (1)
340-368: LGTM! Consistent stub implementation as intended.The MorphTypeData method stubs correctly follow the established pattern for unimplemented CRDT operations in this class. All methods appropriately throw
NotImplementedException, which aligns with the PR objectives stating that CRDT API implementations are not included in this issue and are planned for future work.backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (4)
86-113: LGTM! Comprehensive GUID to MorphType conversion.The pattern matching approach correctly handles all known MorphType values from MoMorphTypeTags. The fallback to
MorphType.Otherfor unrecognized GUIDs andMorphType.Unknownfor null values provides proper error handling.
115-142: LGTM! Reverse conversion with appropriate null handling.The conversion correctly maps MorphType enum values back to their corresponding GUIDs. The comment on line 139 appropriately documents the non-round-trip behavior for
MorphType.Other, which is the expected behavior sinceOtherrepresents unrecognized GUIDs that we cannot map back.
209-235: LGTM! Correct morph type categorization for lexeme form creation.The
IsAffixMorphTypemethod correctly identifies which morphological types should use the affix factory versus the stem factory. The categorization aligns with linguistic principles where circumfixes, infixes, prefixes, suffixes, and their variants are treated as affixes, while everything else uses the stem factory.
237-246: LGTM! Robust entry creation with morph type awareness.The updated
CreateEntrymethod properly integrates MorphType into the lexeme form creation process. The fallback toMorphType.Stemwhen conversion returns null (line 243) provides a safe default, and the non-null assertion is justified sinceToLcmMorphTypeId(MorphType.Stem)will never return null.backend/FwLite/MiniLcm/SyncHelpers/MorphTypeDataSync.cs (4)
1-16: LGTM! Standard collection synchronization pattern.The collection sync method correctly uses the
DiffCollection.Diffutility with a customMorphTypeDataDiffApito handle bulk synchronization. This follows the established pattern used by other sync helper classes in the codebase.
18-25: LGTM! Efficient individual object synchronization.The method correctly computes the diff between before and after states, only performing an update if changes are detected. The return value properly reflects whether any changes were made.
27-50: LGTM! Comprehensive property diff generation.The diff method correctly handles all
MorphTypeDataproperties using appropriate diff utilities:
MultiStringDifffor Name, Abbreviation, and DescriptionSimpleStringDifffor LeadingToken and TrailingTokenIntegerDifffor SecondaryOrderThe early return when no operations are generated (line 48) optimizes performance by avoiding unnecessary updates.
52-70: LGTM! Proper implementation of collection diff API.The nested
MorphTypeDataDiffApiclass correctly implements the required interface methods:
AddcallsCreateMorphTypeDatafor new itemsRemovecallsDeleteMorphTypeDatafor deleted itemsReplacedelegates to the individual sync method for modified itemsThe implementation follows the established pattern and correctly propagates the API calls.
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (5)
491-505: LGTM! Morph type retrieval methods follow established patterns.The
GetAllMorphTypeDataandGetMorphTypeDatamethods correctly implement the standard repository pattern used throughout the class, with appropriate async enumerable collection handling and nullable single-item retrieval.
507-520: LGTM! Comprehensive morph type conversion implementation.The
FromLcmMorphTypeconversion method correctly maps all necessary properties from the LCM morph type to the MiniLCM model, following established patterns for multilingual string handling and enum conversion.
522-553: LGTM! CRUD operations correctly implement FwData project constraints.The implementation correctly handles the constraint that morph types are fixed in FwData projects:
- Create and Delete operations silently ignore requests (as documented in PR objectives)
- Update operations properly implement the undoable unit of work pattern
- The sync-based update method follows established architectural patterns
The design choice to silently ignore rather than throw exceptions aligns with the stated requirements.
600-600: LGTM! Entry morph type integration implemented correctly.The addition of the
MorphTypeproperty to entry creation correctly uses the primary morph type and the established conversion helper. The TODO comment appropriately flags the consideration needed for entries with mixed morph types in future iterations.
904-904: LGTM! Entry creation correctly integrates morph type.The modification to pass
entry.MorphTypeto theCache.CreateEntrymethod correctly integrates morph type support into the entry creation process, aligning with the PR objective to enable creation of entries with the correct morph type.
The "add" operation, if we had ever used it, would have added nothing because the value to be added wasn't being passed in. Fixed now.
github-actions
bot
added
the
📦 Lexbox
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateMorphTypeDataProxy.cs
Outdated
Show resolved
Hide resolved
|
I pushed a bunch of fixes. |
myieye
force-pushed
the
feat/minilcm-morph-types
branch
from
ccd7cf9 to
c95af0c
Compare
|
My commits basically only fixed tests (by fixing/completing functionality). There's just one failing test left, which is sort of related to the fact that
I think this PR has covered at least some of #1847. |
|
@myieye wrote:
Discussed it with Kevin today, and he said "Just exclude it from the test for now, register it once CRDT supports it". So I pushed commit 31218de to do just that. |
3 participants
Fixes #1846.
Implemented: