Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRDT sync handles parts of speech #1203

Merged
merged 9 commits into from
Nov 7, 2024
Merged

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Nov 1, 2024

Fix #1178.

Parts of speech now sync both ways in FW and CRDT sync. Someday we may want to generalize this to include more types of CmPossibilityLists but for now parts of speech are working.

Semantic domains will be handled in a separate PR.

@rmunn rmunn self-assigned this Nov 1, 2024
@rmunn rmunn force-pushed the feat/crdt-sync-parts-of-speech branch from 9e6098d to ff28b54 Compare November 5, 2024 05:43
Copy link
Contributor Author

@rmunn rmunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several bits of this code I would like to discuss with @hahn-kev before merging the PR. But the new unit tests are passing, and the code looks generally all right to me.

backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs Outdated Show resolved Hide resolved
backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs Outdated Show resolved Hide resolved
backend/FwLite/MiniLcm/InMemoryApi.cs Outdated Show resolved Hide resolved
backend/FwLite/MiniLcm/Models/PartOfSpeech.cs Show resolved Hide resolved
@rmunn rmunn requested a review from hahn-kev November 5, 2024 07:33
@rmunn rmunn marked this pull request as ready for review November 5, 2024 07:33
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, just some minor changes, left a comment to start a discussion about if we want to even keep around the in memory implementation.

@rmunn rmunn force-pushed the feat/crdt-sync-parts-of-speech branch from 3336f16 to c364294 Compare November 5, 2024 09:10
@rmunn rmunn requested a review from hahn-kev November 6, 2024 05:10
@rmunn
Copy link
Contributor Author

rmunn commented Nov 6, 2024

@hahn-kev - All requested changes are done.

Only thing I'm not sure of is the CreatePartOfSpeech(input) implementation for the dry run API. I have it just returning the input and not trying to call api.GetPartOfSpeech(input.Id) since that's almost certain to return null. However, I don't know if that's always going to be the case: I could see there being a situation, in certain tests perhaps, where CreatePartOfSpeech is being called in a situation where the api already has a PoS with that GUID.

So perhaps I should have it return api.GetPartOfSpeech(input.Id) ?? input instead. Thoughts?

@hahn-kev
Copy link
Collaborator

hahn-kev commented Nov 6, 2024

I'm fine with returning the input as that's what we do in the other APIs

@rmunn rmunn force-pushed the feat/crdt-sync-parts-of-speech branch from 0013457 to b42959d Compare November 6, 2024 08:51
@rmunn
Copy link
Contributor Author

rmunn commented Nov 6, 2024

@hahn-kev - I've rebased on top of develop and fixed the merge conflicts. I did an interactive rebase so that I could edit the commits that added the PartOfSpeechSync code, and I rewrote them to use their own PartOfSpeechSync.cs static class similar to EntrySync etc.

Tests are still passing. Everything should be ready to re-review and merge.

hahn-kev
hahn-kev previously approved these changes Nov 7, 2024
We've decided to get rid of it, and it's a merge conflict for it to
still exist here.
@rmunn rmunn merged commit 253403b into develop Nov 7, 2024
6 checks passed
@rmunn rmunn deleted the feat/crdt-sync-parts-of-speech branch November 7, 2024 09:54
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.

CRDT sync parts of speech
2 participants