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

Add custom storage #315

Merged
merged 3 commits into from
Sep 9, 2021
Merged

Add custom storage #315

merged 3 commits into from
Sep 9, 2021

Conversation

agraven
Copy link
Contributor

@agraven agraven commented Aug 25, 2021

Closes #271.

Add an additional tree to the Store where custom api consumer data may
be stored.

I'm not sure exactly what types are the smartest to use for the keys and values here. While it would be nice to be able to use sled-types as suggested by the issue, it would make the custom store implementation dependent. I'm also not sure if the method on Client is necessary when the custom store can be interacted with via getting a reference to the Store.

Add an additional tree to the Store where custom api consumer data may
be stored.
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

I don't think there's anything wrong with using &[u8] for the types, people will probably just serialize stuff and using &[u8] and Vec<u8> seems to be the most flexible way to handle this.

I think removing the methods from the Client should be fine.

@@ -828,6 +828,16 @@ impl Client {
self.base_client.store()
}

/// Store a value in the custom tree on the store
pub async fn store_save_value(&self, key: &[u8], value: Vec<u8>) -> Result<Option<Vec<u8>>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

As you noticed, you can get the Store from the client, so these methods are a bit redundant. I would say let's try to keep the Client slim, it is a bit an impossible task to do so considering the API surface of Matrix but removing this certainly would help with that.

@poljar poljar merged commit b09b667 into matrix-org:master Sep 9, 2021
@poljar
Copy link
Contributor

poljar commented Sep 9, 2021

Made the necessary changes myself. Thanks, merged.

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.

Expose custom storage
2 participants