Skip to content
This repository has been archived by the owner on Mar 18, 2022. It is now read-only.

draft: add TYPE command #37

Closed
wants to merge 4 commits into from
Closed

draft: add TYPE command #37

wants to merge 4 commits into from

Conversation

tbmreza
Copy link
Contributor

@tbmreza tbmreza commented May 11, 2021

How do you test a new command? I'm not sure if I'm already on the right direction, i.e., if I only need to figure out what to feed send_packed_command() with, and how to receive the connection's response.

#28

@evoxmusic
Copy link
Contributor

If a command is not available via the Redis Rust client you can use send_packed_command(...) - is it correct @clarity0? I didn't have the time to try this command since I am working on the raft implementation

@clarity0
Copy link
Contributor

@tbmreza I havent tested it out extensively but according to the redis-rs docs changes the state of the connection or something, and my test for the GETDEL command only behaved as expected when I called get twice. I just gave up testing that out because no Redis client I've seen implements GETDEL and it's deprecated apparently. see my comment here

@clarity0
Copy link
Contributor

clarity0 commented May 11, 2021

I think the python redis client does implement TYPE unlike redis-rs. You could test it out with that, because send_packed_command just ignores the response from the server.

Comment on lines 117 to 126
fn value_type(&mut self, key: &[u8]) -> String {
if let Some(data_type) = self.data_mapper.get(key) {
match data_type {
DataType::String => format!("string"),
DataType::List => format!("list"),
DataType::Set => format!("set"),
DataType::Hash => format!("hash"),
}
} else {
format!("none")
Copy link
Contributor

Choose a reason for hiding this comment

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

better to use "string".to_string() here since u don't actually need to format

Copy link
Contributor

Choose a reason for hiding this comment

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

Also you can match directly on the Some inner type like Some(DataType::String) => ... , that way you can match directly on the datamapper.get and avoid nesting it like that with an if let. Also I think it's better if you return a &str from value_type, that way you don't need to allocate a whole String

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@clarity0 Got it. I have just:

  • changed the return type to &str
  • matched directly without if let nest
  • added the command in the python client

Is that everything?

@tbmreza tbmreza marked this pull request as ready for review May 12, 2021 00:55
@clarity0
Copy link
Contributor

Does it work well with the python client?

@tbmreza
Copy link
Contributor Author

tbmreza commented May 12, 2021

Does it work well with the python client?

Should I test it thoroughly? If so, do I put them (SET and TYPE commands for every DataType) all in main.py? @clarity0

@tbmreza
Copy link
Contributor Author

tbmreza commented May 12, 2021

for the 1 TYPE command I tested in python client, yes it works well (the assertion doesn't fail)

@clarity0
Copy link
Contributor

Right now there is only support commands that add strings so you can't really test the other cases with a client but they should work fine.

@evoxmusic
Copy link
Contributor

@clarity0 @tbmreza good to be merged?

@evoxmusic
Copy link
Contributor

@tbmreza can you rebase?

@tbmreza
Copy link
Contributor Author

tbmreza commented May 14, 2021

@tbmreza can you rebase?

sure but I'm not a capable git user. could you tell me how to do it, step by step?

@tbmreza
Copy link
Contributor Author

tbmreza commented May 22, 2021

I'm closing this PR. Currently looking at other good first issues (#23 if it doesn't turn out to be too hard for me), will open new PR covering that and #28. Thanks for bearing with me!

@tbmreza tbmreza closed this May 22, 2021
@tbmreza tbmreza deleted the type-command branch May 22, 2021 22:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants