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 support to delete hash fields #130

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

prashanthpai
Copy link
Contributor

No description provided.

// see https://github.com/redis/redis/issues/7860

unsafe {
RedisModule_HashSet.unwrap()(
Copy link
Contributor Author

@prashanthpai prashanthpai Mar 2, 2021

Choose a reason for hiding this comment

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

It seems like the RedisModule_HashSet C API has been overloaded with multiple things and it does a lot more than merely setting a hash value.

I'm very new to Rust. I don't know where to draw the line between this project providing a user-friendly Rust API to modules vs this project being a very thin and pass-through wrapper around the C modules API. Does it make sense to extend hash_set to take a additional flags/options instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The aim of this module is to provide an idiomatic Rust API for the Redis modules API. Using flag arguments to fundamentally change the functionality of a method isn't very common even in C, doubly so in Rust; so I think the approach you took here is the right one.

One thing I would change is rename all the instances of hash_del_single to simply hash_del, to match the existing style of hash_set/hash_get.

Oh, and thank you for the PR!

src/raw.rs Outdated
Comment on lines 355 to 356
// support to pass multiple fields is desired but it's complicated
// see https://github.com/redis/redis/issues/7860
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed; if we want to support multiple fields we should use a macro-based solution like we did for hash_get_multi.

@@ -156,6 +156,7 @@ extern "C" {
///////////////////////////////////////////////////////////////

pub const FMT: *const c_char = b"v\0".as_ptr() as *const c_char;
pub const REDISMODULE_HASH_DELETE: *const RedisModuleString = 1 as *const RedisModuleString;
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to figure out why this was needed -- I expected bindgen to generate this automatically.
It turns out it doesn't because it can't parse the typecast, see rust-lang/rust-bindgen#316.
Please add a comment here that mentions this.

@prashanthpai prashanthpai force-pushed the add-hash-del-single branch from d6c427f to 3e8b9e6 Compare March 3, 2021 17:32
@prashanthpai
Copy link
Contributor Author

@gavrie @slavak Thank you for the quick review. I've updated the PR.

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