Conversation
CRDT CRUD operations for MorphTypeData are not yet implemented, so this will fail to sync and throw a NotImplementedException if you actually try to sync.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request adds comprehensive MorphTypeData support to the CRDT system, including database migrations, change tracking, API implementations, sync logic, and test infrastructure. Key features include unique constraint enforcement on MorphType, immutability of MorphType post-creation, duplicate prevention, and nullable LeadingToken/TrailingToken properties. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
github-actions
bot
added
the
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
In liblcm, the .Prefix and .Postfix properties can be (and often are) null, so the corresponding LeadingToken and TrailingToken properties in MorphTypeData need to be nullable as well. The UI will have to do ?? "" in appropriate places as it displays the leading/trailing tokens.
rmunn
changed the title
Created the change but forgot to include it in what the method returns.
Test was failing because mock didn't have new API registered so was returning null.
rmunn
force-pushed
the
feat/sync-morph-types
branch
from
8003c78 to
7563c10
Compare
This one used yet another snapshot file that I hadn't updated yet
|
Kind of bothered by the fact that I don't see FwDataApiDoesNotUpdateMorphType failing yet; that test is supposed to start failing once we start syncing morph type. The fact that it's not failing yet may mean I've failed to implement something. |
This time instead of an empty list of morph types, we'll use the actual morph types found in the .fwdata file, which is what the test will actually see.
Needed both the Snapshot *and* the sqlite3 database to be updated to expect the new MorphTypeData table.
Check in two places: the API, which is the most likely place to catch duplicates, but also in the Change object itself, in case two people somehow end up submitting the same change twice (e.g., one person was offline for a period of time).
|
Okay, tests are finally green, so I can finally push the "don't create duplicate MorphTypes" logic and make sure it doesn't break the tests. |
Now that the API prevents creating duplicate MorphTypes, we can add a database constraint to enforce that at the DB level.
github-actions
bot
added
the
📦 Lexbox
Needs to match the C# name since we autogenerate the Typescript types from the C# types.
Missed these the first time around
|
I suspect I need to change the JSON-encoded Sena-3 data in demo-entry-data.ts, but that's going to be a bit of a pain and I don't want to do it right now. I'll do that on Monday. |
Will need to rename some new entries in the demo data from develop, which are causing the tests to fail because they have the old names.
|
Turns out the test failures were because of a use of the MorphType enum in |
This allows us to not mark them nullable, even though the parameterless constructor doesn't set them, pushing the burden of setting those properties off onto the calling code that creates the object.
The CreateMorphTypeChange main constructor sets all required properties, but C# needs a compiler hint to tell it so.
|
@myieye - Fixed the CreateMorphTypeChange type by using Did you hear from Jason about renaming LeadingToken and TrailingToken? I'm about to do that rename (once I verify that the tests have turned green again), then this should be ready for final approval. I tagged you for re-review already, but if you want to hold off until I push the LeadingToken and TrailingToken rename, that's fine. P.S. I plan to do the rename of LeadingToken and TrailingToken in a single commit, which I will get running locally and then push. That way it will be easy to revert if we decide later that we don't want to match the FwData names. But it seems likely that we'll want to match FwData's names internally, so I'll go ahead with the rename before hearing what Jason thinks¸ as the work isn't likely to be wasted. |
Matches the names from liblcm, which may help reduce confusion.
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
I think this is almost there. 👍
I just noted the validation changes we agreed on that you haven't done yet.
Also, I think we should not merge this. Instead I've enabled the fw-lite CI actions to run on PRs against this branch. So, we'll merge in a bunch of other morph-type stuff like:
- tests
- my morph-token PR
- adding canonical morph-types to existing crdt projects (see new sub-issue)
- adding an interceptor for updating headwords on morph-type changes (see new sub-issue)
- etc.
I think we want at least all of those as a "bundle" or else we can't release.
Validation! Slaps forehead. I was thinking yesterday "I know there's one more thing but I don't remember what it is." :-) |
Since morph types can never be created or deleted in FW, the 19 canonical morph types are the only ones we'll ever encounter. The Unknown value is good enough for those rare cases where we need a different value, so Other is a corner case ripe for bugs.
Test is skipped for now because we need to wrap the validation wrapper around the API before the test will throw the exception it's supposed to throw; currently it throws a DB constraint violation, which is not actually what we want to be throwing.
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
Arg, I forgot to submit this feedback, sorry 🙁
|
|
||
| var prefixingInterfix = await crdtApi.GetMorphType(MorphTypeKind.PrefixingInterfix); | ||
| prefixingInterfix.Should().NotBeNull(); | ||
| await crdtApi.DeleteMorphType(prefixingInterfix.Id); |
There was a problem hiding this comment.
Actually, we probably shouldn't have a delete-morph-type method, should we?
I don't think that makes any sense.
In fact...hmm...seeing as we're going to create morph-types independently from a fw-headless sync...we don't actually have any good reason to have a create-morph-type method either, except for our own internal purposes of adding the initial morph-types.
I think we should really remove both delete and create from the MiniLcmApi interface.
There was a problem hiding this comment.
I've written Jason a message to ask if we need to anticipate FLEx projects with non-standard morph-type lists. (different GUIDs, missing morph-types, extra morph-types etc.)
Even though the UI prevents stuff like that...almost anything is possible in theory.
There was a problem hiding this comment.
His reply:
After some research the surprising (to me) answer is that the morpheme type list has been static in number and guid since it was introduced into the project template. Currently users can not modify that list. So I think that you can make that assumption. The consequences of some historical bug or feature that allowed the list in a project to be modified would be that any deleted items from the template would appear (no real problems) and that custom items would be deleted (big problems). So I would recommend at least adding a DeleteMorphType operation that throws an ugly exception for user safety.
There are a couple places we could add an "ugly exception":
- When we map a LibLCM entry to a MiniLCM entry and find a non-null and non-canonical morph-type
- When we're diffing morph-types while syncing and find a morph-type that needs to be added or deleted
What are your thoughts on this?
Ultimately creating morph-types definitely doesn't make sense, because we prepopulate them in the DB and the enum column is both required and unique, so it's impossible to create a new one.
There was a problem hiding this comment.
We'll do this in a follow up PR. Until we're auto-adding canonical morph-types tests will throw if we implement this.
They don't, because we need to accept essentially all FLEx data.
* Add filtering on token aware headwords * Respect SecondaryOrder when sorting * Make FTS Headword column contain morph-tokens and all vernacular WS's
* Initial work on morph types in UI Morph types now show leading/trailing tokens in headword, but do not yet have a dropdown for editing them in the entry UI. * Citation forms should not be decorated Lexeme forms should be decorated with prefix/postfix tokens according to the morph type, but citation forms are meant as "overrides" and should be reproduced exactly as-is, without morph type tokens. This is the rule used by FLEx for how it displays words, so FW Lite should do the same. As a bonus, there is now only one `headword` function in the writing system service, instead of two functions with the same name that did two slightly different things. * Fix tests --------- Co-authored-by: Tim Haasdyk <tim_haasdyk@sil.org>
2 participants
Fixes #1847.