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

clone list of revertibles #23424

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jikim-msft
Copy link
Contributor

Description

13865

This PR adds a helper function that takes in the list of revertibles and returns the cloned revertibles in order.

@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 Jan 2, 2025
packages/dds/tree/src/shared-tree/treeCheckout.ts Outdated Show resolved Hide resolved
packages/dds/tree/src/test/utils.ts Outdated Show resolved Hide resolved
packages/dds/tree/src/test/utils.ts Outdated Show resolved Hide resolved
packages/dds/tree/src/test/utils.ts Outdated Show resolved Hide resolved
@jikim-msft jikim-msft marked this pull request as ready for review January 6, 2025 17:57
@jikim-msft jikim-msft requested a review from a team as a code owner January 6, 2025 17:57
* @param targetBranch - The target branch to clone the revertibles for
* @returns Array of cloned revertibles, maintaining the same order as the input
*/
export function cloneRevertiblesInBatch(
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be package exported for users to be able to use it

Copy link
Contributor

Choose a reason for hiding this comment

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

(with the alpha tag)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made changes while you were reviewing 😄. Would you look into this again please?

@github-actions github-actions bot added the public api change Changes to a public API label Jan 6, 2025
@github-actions github-actions bot added the area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct label Jan 7, 2025
Copy link
Contributor

github-actions bot commented Jan 7, 2025

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170492 links
    1603 destination URLs
    1838 URLs ignored
       0 warnings
       0 errors


Comment on lines +689 to +692
const batchedRevertibles: RevertibleAlpha[] = [];
for (const revertible of undoStack) {
batchedRevertibles.push(revertible);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to just do const batchedRevertibles = [...undoStack];

Comment on lines +617 to +624
// Check if the original branch and forked branch have a common ancestor.
const originalBranchHead = this.#transaction.activeBranch.getHead();
const forkedBranchHead = forkedCheckout.#transaction.activeBranch.getHead();

assert(
findCommonAncestor(originalBranchHead, forkedBranchHead) !== undefined,
0x576 /* branch A and branch B must be related */,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be done in a separate PR, and it should throw a UsageError instead of asserting.

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 area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct 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.

5 participants