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.

Sign up for GitHub

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

Jump to bottom

Feat/1246 Allow reordering senses #1308

Merged
merged 29 commits into from
Dec 18, 2024
Merged

Feat/1246 Allow reordering senses #1308

merged 29 commits into from
Dec 18, 2024

Conversation

myieye
Copy link
Contributor

@myieye myieye commented Dec 6, 2024

The first part of #1246 (senses)

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 6, 2024 •
edited
Loading

UI unit Tests

12 tests   12 ✅  0s ⏱️
 4 suites   0 💤
 1 files     0 ❌

Results for commit 42a0418.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 6, 2024 •
edited
Loading

C# Unit Tests

103 tests   103 ✅  5s ⏱️
 16 suites    0 💤
  1 files      0 ❌

Results for commit 42a0418.

♻️ This comment has been updated with latest results.

hahn-kev
hahn-kev requested changes Dec 9, 2024
View reviewed changes
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.

quick review, I didn't dig into the details yet.

backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs Outdated Show resolved Hide resolved
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs Outdated Show resolved Hide resolved
backend/FwLite/LcmCrdt/OrderPicker.cs Outdated Show resolved Hide resolved
backend/FwLite/LcmCrdt/QueryHelpers.cs Outdated Show resolved Hide resolved
backend/FwLite/LcmCrdt/QueryHelpers.cs Outdated Show resolved Hide resolved
backend/FwLite/MiniLcm/Models/Sense.cs Outdated Show resolved Hide resolved
backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs Outdated Show resolved Hide resolved
myieye added 15 commits December 10, 2024 16:51
@myieye
Add collection position diffing
d8cb48b
@myieye
Add some collection diff tests and redesign to use after/before posit…
62864b2 
…ioning
@myieye
Exclude Order from external serialization, refactor, add more tests
80850a7
@myieye
Update/extend entry tests for sense reordering and cleanup
df2a15e
@myieye
Rename method
028203f
@myieye
Don't set order on moved sense, because it shouldn't be necessary to …
7df8aea 
…set an internal property for an external caller.
@myieye
Set sense order when creating entries in bulk
1726b5a
@myieye
Standardize using AllSenses (not SensesOS) and fix moving between sub…
9d7b850 
…senses
@myieye
Cleanup
c5f30ca
@myieye
Fix tests
970a43c
@myieye
Revert making Sense partial
b3bf2be
@myieye
Sort senses in place
5f5a492
@myieye
Refactor OrderPicker to require siblings
a87b7ba
@myieye
Rename BetweenPosition props and simplify orderable methods
cebb3d5
@myieye
Refactor collection diffing to use class/object for add, delete, move…
b82d56b 
… & replace
@myieye myieye force-pushed the feat/1246-allow-reordering-senses-example-sentences-complex-forms-and-components branch from 70ff1be to b82d56b Compare December 10, 2024 15:52
@hahn-kev hahn-kev mentioned this pull request Dec 11, 2024
Open
hahn-kev
hahn-kev requested changes Dec 11, 2024
View reviewed changes
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.

left some feedback, I also pushed a couple of minor changes

backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs Outdated Show resolved Hide resolved
backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs Outdated Show resolved Hide resolved
backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs Show resolved Hide resolved
backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs Outdated Show resolved Hide resolved
backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs Show resolved Hide resolved
backend/FwLite/LcmCrdt/OrderPicker.cs Outdated Show resolved Hide resolved
backend/FwLite/LcmCrdt/OrderPicker.cs Outdated Show resolved Hide resolved
backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt Outdated Show resolved Hide resolved
backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs Outdated Show resolved Hide resolved
backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs Outdated Show resolved Hide resolved
@hahn-kev hahn-kev mentioned this pull request Nov 29, 2024
Open
3 tasks
@myieye myieye requested a review from hahn-kev December 16, 2024 10:34
hahn-kev
hahn-kev approved these changes Dec 17, 2024
View reviewed changes
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 to me, I left some questions but I'm fine if this is merged in as is. I would like some tests on the TestOrderableDiffApi but I'm fine if that's in a separate PR if you just want to get this merged in

@myieye myieye merged commit 7ff7676 into develop Dec 18, 2024
21 of 22 checks passed
@myieye myieye deleted the feat/1246-allow-reordering-senses-example-sentences-complex-forms-and-components branch December 18, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hahn-kev hahn-kev hahn-kev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@myieye @hahn-kev