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

Publish snapshot map changelog #622

Merged
merged 7 commits into from
Jan 5, 2022
Merged

Publish snapshot map changelog #622

merged 7 commits into from
Jan 5, 2022

Conversation

maurolacy
Copy link
Contributor

Related to #600 and #621.

To allow prefixing / iteration over the changelog entries.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

LGTM.
Does this solve your issue @ben2x4 ?

@maurolacy
Copy link
Contributor Author

maurolacy commented Jan 4, 2022

Tried to extend this to SnapshotItem as well, but hit some issues with Unit () deserialization. I think this is good enough for now, and we can extend to item only when / if needed.

@maurolacy
Copy link
Contributor Author

Tried to extend this to SnapshotItem as well, but hit some issues with Unit () deserialization. I think this is good enough for now, and we can extend to item only when / if needed.

But see latest version.

@maurolacy maurolacy force-pushed the snapshotmap-changelog branch from 50ac360 to 1379d16 Compare January 4, 2022 15:03
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Makes sense. Minor style comment


pub fn changelog(&self) -> Map<u64, ChangeSet<T>> {
// Build and return a compatible Map with the proper key type
Map::new(unsafe { from_utf8_unchecked(self.snapshots.changelog.namespace()) })
Copy link
Member

Choose a reason for hiding this comment

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

Why not expose this functionality as a method on Snapshot? (self.snapshots.changelog())

Copy link
Contributor Author

@maurolacy maurolacy Jan 5, 2022

Choose a reason for hiding this comment

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

No, because I'm overriding the changelog map key type here. Our empty (Unit) keys have a particular behaviour, in that they are not length-prefixed zero-length elements in tuples (they are just being skipped, when building the key).

Anyway, that's another issue, and (in this case) due to an implementation detail. What I can do to improve on this is to save the changelog namespace in this class, so that I can use it here directly, and avoid the re-conversion to string. Will do.

@ben2x4
Copy link

ben2x4 commented Jan 4, 2022

LGTM. Does this solve your issue @ben2x4 ?

Yes, I like this approach. Give the user direct access to the changelog without making the whole underlying snapshot item public, very good compromise between the two approaches I suggested.

Thanks so much @maurolacy!

@maurolacy maurolacy force-pushed the snapshotmap-changelog branch from 25d37ad to 011fa15 Compare January 5, 2022 07:36
@maurolacy maurolacy force-pushed the snapshotmap-changelog branch from 011fa15 to a797917 Compare January 5, 2022 07:40
@maurolacy maurolacy merged commit 501efe8 into main Jan 5, 2022
@maurolacy maurolacy deleted the snapshotmap-changelog branch January 5, 2022 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants