-
Notifications
You must be signed in to change notification settings - Fork 40
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
Workspaceops rework read folder #7107
Conversation
417d805
to
c796879
Compare
3baf1dc
to
d02717a
Compare
c614c82
to
d02717a
Compare
d02717a
to
2499622
Compare
2499622
to
d02717a
Compare
…paceOps::stat_folder_children/open_folder_reader_by_id//open_folder_reader for that
410d1e0
to
4fb51fe
Compare
4fb51fe
to
ac26c38
Compare
… `key` field from `BlockAccess`
…Parsec v2 is no longer needed)
…at with Parsec v2 is no longer needed)
be48743
to
2578c32
Compare
// 4) Merge data and ensure the sync is still needed | ||
|
||
// Destruct local and remote manifests to ensure this code with fail to compile | ||
// whenever a new field is introduced. |
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.
I don't quite get why you would want that ? Is it to have a real human person check that the new field does not interact with the merging process, or to adapt the merging process ?
Maybe annotating LocalFileManifest with non_exhaustive could do the trick
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.
Consider the following:
struct Data {
updated: DateTime,
}
fn merge(local: Data, remote: Data) -> Data {
remote.updated = max(local.updated, remote.updated);
remote
}
Then we introduce a new created
field
struct Data {
created: DateTime,
updated: DateTime,
}
The code will compile fine event if we haven't updated merge
(typically we should have added a line like remote.created = min(local.created, remote.created)
).
This is an issue given this merge
function is easy to forget to update:
- it is in a different file
- its tests will most likely just ignore the new field
On the other hand, if the merge
function is implemented with exhaustive de-structuring, it will fail to compile any time a new field is added. Hence forcing the developer to have a look at this part of the code.
This is a pattern we also use in the CRC code of the testbed. Nobody knows about this piece of code and nobody cares... but if a structure changes and the CRC starts being wrong, we will have very hard time understanding why the tests start becoming randomly wrong (as client and testbed server code may end up with different data with the same CRC...)
Regarding non_exhaustive
, its point is to forbid exhausting destructuring, which is the opposite of what we want here ;-)
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.
Thanks, so we could put that behind a debug_assert to make sure that it won't be present during runtime ?
…s `WorkspaceOps::rename_entry`
…rkspaceOps::move_entry`)
e77090b
to
e96906a
Compare
Closes #5820
Closes #6491
Closes #6914