feat: implement INC - #2
Conversation
|
Oh you really revived this project haha, I'll take a look, thanks : ) |
theopfr
left a comment
theopfr
left a comment
There was a problem hiding this comment.
Thanks for the PR! Added a couple of small comments but all-in-all nice job : )
| assert_eq!(get_query, parser_error!(ParserErrorType::UnexpectedCharacter)); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
Please add some tests here similiar to the others.
| let response_inc = state.execute_request(client_address, QueryRequest::INC("key_inc".to_string())); | ||
| assert_eq!(response_inc, Ok(QueryResponseType::INC_OK(ValueType::Int(1)))); | ||
|
|
There was a problem hiding this comment.
Coud you add the same query again to test if the value was incremented to 2?
| - [x] add graceful shutdown | ||
| - [x] add command line flag handler | ||
| - [x] built CLI client | ||
| - [x] add INCR/DECR commands for INT type |
There was a problem hiding this comment.
Please add an entry to the Commands table in the README for the new INC command : )
| let default_value = ValueType::Int(1); | ||
| self.storage.insert(key.to_owned(), serialize(&default_value).unwrap()); | ||
| return Ok(QueryResponseType::INC_OK(default_value)); | ||
| } |
There was a problem hiding this comment.
Nice! Two points here:
- We need a guard here at the beginning of the function that checks if the value-type is INT. Because if it the type is STR and we do
INC keyfor a non-exisiting key, it will create a key-value pair "key: 1" where 1 is an integer and not a STR. - Could you add also tests here in the
database.rsforinc?
There was a problem hiding this comment.
How do I check if the value is int? I thought about it but it's not very clear to me.
There was a problem hiding this comment.
Sorry, I meant check the database-type, so it only works when the database stores INTs, e.g like this:
if self.database_type != DatabaseType::Int {
return database_error!(DatabaseErrorType::WrongValueType);
}| }, | ||
| QueryResponseType::INC_OK(value) => { | ||
| Self::build_ok_response("INC".to_string(), Some(Self::handle_value_types(&value)), Some(database_type)) | ||
| } |
There was a problem hiding this comment.
Also add tests for inc in this file pls
2 participants
I added this draft to gather feedback. Lmk what you think @theopfr and I can implement also
DECR.