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

tree2: add sequence mutators #17523

Merged
merged 15 commits into from
Sep 29, 2023
Merged

Conversation

taylorsw04
Copy link
Contributor

This PR implements sequence mutation methods. Tests will be added in a follow-up PR when #17475 is complete.

@taylorsw04 taylorsw04 requested a review from a team as a code owner September 27, 2023 23:18
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: tree base: main PRs targeted against main branch labels Sep 27, 2023
* @param index - The index at which to insert `value`.
* @param value - The content to insert.
*/
insertAt(index: number, value: FlexibleNodeContent<TTypes>[]): void;
Copy link
Contributor

@Josmithr Josmithr Sep 28, 2023

Choose a reason for hiding this comment

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

What are the merge semantics here? I think it would be valuable to call them out (here and in other methods as appropriate). Are we inserting after the previous item? Before the following item?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither; all methods shipped as part of MVP are index-centric, so they will result in insertion at the index specified unless concurrent changes push them forward (first-write-wins). When we introduce anchored changes, we can add those docs, but for now it's intuitive enough to omit (I validated this opinion with Noah/Alex).

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. That's probably fair. Still might be worth noting that. If I were looking at this API (as I was as a reviewer), that is one of the first questions I would be asking.

@github-actions github-actions bot added the public api change Changes to a public API label Sep 28, 2023
@Josmithr
Copy link
Contributor

Is the intention to add unit tests for the new behaviors in this PR, or separately?

* @param sourceEnd - The ending index of the range to move (exclusive)
* @throws Throws if any of the input indices are invalid.
* @remarks
* All indices are relative to the sequence excluding the nodes being moved.
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem user friendly IMO. Unless I'm missing something. The consumer is required to do the necessary offset math to figure out what index would be if the selected items were not in the list. Couldn't we do that math for them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, we are planning to do that at a lower level (i.e. inside of the sequence editor code). If that doesn't land by MVP, we can do the math at this higher layer, I suppose.

@msfluid-bot
Copy link
Collaborator

@fluid-example/bundle-size-tests: +956 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 443.96 KB 443.96 KB No change
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 237.29 KB 237.29 KB No change
loader.js 148.39 KB 148.39 KB No change
map.js 48.43 KB 48.43 KB No change
matrix.js 142.21 KB 142.21 KB No change
odspDriver.js 90.46 KB 90.46 KB No change
odspPrefetchSnapshot.js 42.13 KB 42.13 KB No change
sharedString.js 162.86 KB 162.86 KB No change
sharedTree2.js 257.88 KB 258.81 KB +956 Bytes
Total Size 1.68 MB 1.68 MB +956 Bytes

Baseline commit: 14d4581

Generated by 🚫 dangerJS against d6ba598

@DLehenbauer DLehenbauer merged commit 61d83df into microsoft:main Sep 29, 2023
28 checks passed
@DLehenbauer
Copy link
Contributor

@taylorsw04 - I'm in the market for some sequence mutators to write test cases... hope you don't mind my merging. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: tree area: dds Issues related to distributed data structures base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants