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

Removes log from DocumentState #40

Merged
merged 2 commits into from
Dec 12, 2024
Merged

Removes log from DocumentState #40

merged 2 commits into from
Dec 12, 2024

Conversation

mzkrasner
Copy link
Contributor

  • log: [] was previously part of DocumentState's definition in the model-instance-handler packages and corresponding tests

This PR:

  • Removes log from DocumentState definition
  • Updates impacted methods that previously utilized this field
  • Updates tests to remove checks for "links to state" based on log contents
  • Adds getDocumentModel() method to model-client

@mzkrasner mzkrasner requested a review from nathanielc December 11, 2024 22:31
nathanielc
nathanielc approved these changes Dec 12, 2024
View reviewed changes
Copy link
Collaborator

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

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

LGTM with a few small changes.

packages/model-instance-handler/src/assertions.ts Outdated
// `Commit doesn't properly point to previous event payload in log for document ${initCID}. Expected ${expectedPrev}, found 'prev' ${prev}`,
// )
// }
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we delete this code if its not needed anymore? We can always get it back from git it we need it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

packages/model-instance-handler/test/assertions.test.ts Outdated
// `Commit doesn't properly point to previous event payload in log for document ${initID}. Expected ${expectedID}, found 'prev' ${invalidID}`,
// )
// })
// })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, lets just delete the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@mzkrasner mzkrasner merged commit 720f22c into main Dec 12, 2024
10 checks passed
@mzkrasner mzkrasner deleted the feat/remove-log-DocumentState branch December 12, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanielc nathanielc nathanielc 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
@mzkrasner @nathanielc