Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(store/v2): add WorkingHash #20706
feat(store/v2): add WorkingHash #20706
Changes from 7 commits
0cc9a87
bd65ac6
fbbe274
e38630e
ff4aa41
46767e2
faf3033
ee8a324
a23ee4a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @julienrbrt also we should document why this is being set in a small godoc so the next person understands the reasoning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this comment means?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ee8a324#diff-fe112fb8292a1257bb07eda502a059bcb8ff79ce758175f2372acec1f622b0c4R240-R241
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently,
c.cfg
is empty, I am not sure why needs an empty config ❓There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify the use of
commitHeader
in theStore
struct.The comment for
commitHeader
should clarify its exact role, especially how it interacts with other components and any implications it has on the state or behavior of the store.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor the
WorkingHash
method for better error handling and separation of concerns.The
WorkingHash
method should separate the concerns of writing to the SC and SS backends more clearly and ensure that errors from these operations are handled correctly without affecting each other.Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved TestWorkingHash, but consider additional scenarios.
The updated
TestWorkingHash
method now covers scenarios with multiple versions, which is a significant improvement. However, it would be beneficial to include tests where changes are made between calls toWorkingHash
andCommit
to ensure the method behaves as expected in more complex situations.Consider adding tests that modify the state between
WorkingHash
andCommit
calls to fully ensure the robustness of this functionality.Would you like me to help draft these additional test scenarios?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a detailed method comment for
WorkingHash
.The
WorkingHash
method lacks a detailed comment explaining its parameters, return values, and any side effects. This is crucial for understanding the method's impact, especially since it modifies the state.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify the
Commit
method's interaction withWorkingHash
.The comment on the
Commit
method should be expanded to clearly explain how it interacts withWorkingHash
, especially regarding the state changes and the conditions under which the changeset is overwritten.