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

Add Update(before, after) API for senses #1267

Merged
merged 4 commits into from
Dec 6, 2024
Merged

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Nov 25, 2024

Fix #1186.

Adds a new UpdateSense(Guid entryId, Sense before, Sense after) method to the IMiniLcmWriteApi interface.

Also adds a GetSense(Guid entryId, Guid id) method to the IMiniLcmReadApi interface.

@rmunn rmunn requested a review from hahn-kev November 25, 2024 06:32
@rmunn rmunn self-assigned this Nov 25, 2024
Copy link

github-actions bot commented Nov 25, 2024

C# Unit Tests

98 tests   98 ✅  5s ⏱️
15 suites   0 💤
 1 files     0 ❌

Results for commit 10e672a.

♻️ This comment has been updated with latest results.

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.

this looks like a good start, though SenseDiffToUpdateneeds to be changed to remove the code doing the diff on semantic domains and instead in in SenseSync.Sync it needs to do a collection diff and use the apis AddSemanticDomainToSense and the remove variant, if there's any differences between the same domains those should be ignored.

@rmunn rmunn requested a review from hahn-kev November 29, 2024 06:44
@rmunn
Copy link
Contributor Author

rmunn commented Nov 29, 2024

SenseDiffToUpdateneeds to be changed ...

Done in commit 95441bb.

hahn-kev
hahn-kev previously approved these changes Dec 2, 2024
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.

looks good thanks!

@rmunn
Copy link
Contributor Author

rmunn commented Dec 3, 2024

Since merging develop in, there's one test failure: SenseDiffShouldUpdateAllFields is expecting before.SemanticDomains to have 1 item, but it's empty. I don't know if this is caused by a recent change on develop and I need to update the test to exclude SemanticDomains (and probably PartsOfSpeech) from the .BeEquivalentTo() check, or if this is a real bug. @hahn-kev - any ideas?

@hahn-kev
Copy link
Collaborator

hahn-kev commented Dec 3, 2024

@rmunn yes that's expected, you can configure that test to ignore SemanticDomains, that set of tests is really only checking the diff update produced on the basic fields of an object, eg changes which go through Update(id, jsonPatch) not reference changes like Semantic domains

@rmunn
Copy link
Contributor Author

rmunn commented Dec 4, 2024

Note that this PR has the (after, before) ordering in it; that will be fixed by #1303, so we should not change it here lest merge resolution end up doing the wrong thing.

rmunn added 2 commits December 4, 2024 12:35
Rebasing into develop pulled in a change that LfClassicMiniLcmApi didn't
yet implement. Since LF Classic doesn't handle complex forms, the
implementation is dead simple: just return null.
@rmunn rmunn merged commit 58b5064 into develop Dec 6, 2024
16 checks passed
@rmunn rmunn deleted the feat/new-update-api-senses branch December 6, 2024 02:27
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.

implement new update api Senses
2 participants