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

add support for INCRBY command #30

Merged
merged 13 commits into from May 19, 2021
Merged

Conversation

pepoviola
Copy link
Contributor

Hi @evoxmusic, this pr implement "INCRBY" for #3 .
Thx!

redisless/src/server.rs Outdated Show resolved Hide resolved
if let Some(key) = get_bytes_vec(v.get(1)) {
if let Some(increment_bytes) = get_bytes_vec(v.get(2)) {
if let Ok(increment_str) = std::str::from_utf8(&increment_bytes[..]) {
if let Ok(increment) = increment_str.parse::<i64>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does INCRBY should accept i64 instead of u64? Is it allowed to do INCRBY key -10? If yes, can you add a test for that case? Ty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I double check this one because sound weird for me too but

https://redis.io/commands/incrby

64 bit signed integer

I tested and works as expected

127.0.0.1:6379> SET mykey "10"
OK
127.0.0.1:6379> INCRBY mykey 5
(integer) 15
127.0.0.1:6379> INCRBY mykey -5
(integer) 10

And also there is a test :)

let x: u32 = con.get("intkeyby").unwrap();
assert_eq!(x, 20u32);

let _ = con
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evoxmusic INCRBY "-5" test here.

@pepoviola
Copy link
Contributor Author

that weird, test are passing on my side

running 12 tests
test command::tests::set_command ... ok
test protocol::test::test_array_of_arrays ... ok
test protocol::test::test_bulk_string ... ok
test protocol::test::test_arrays ... ok
test protocol::test::test_errors ... ok
test protocol::test::test_nil ... ok
test protocol::test::test_simple_string ... ok
test server::tests::start_and_stop_server_multiple_times ... ok
test server::tests::setex_and_expire ... ok
test server::tests::test_redis_implementation ... ok
test server::tests::start_and_stop_server ... ok
test tests::start_and_stop_server_from_c_binding ... ok

test result: ok. 12 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 6.51s

   Doc-tests redisless

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

I will check.

@evoxmusic
Copy link
Contributor

Let me know if you need help

@pepoviola
Copy link
Contributor Author

Let me know if you need help

Hi @evoxmusic, can you try to run this branch locally ?

Thx!

This is the error in GH Actions.

thread 'server::tests::test_redis_implementation' panicked at 'called `Result::unwrap()` on an `Err` value: Response was of incompatible type: "Response type not convertible to numeric." (response was ok)', src/server.rs:428:42
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@evoxmusic
Copy link
Contributor

@clarity0 looks good for you? (it looks good to me)

@pepoviola
Copy link
Contributor Author

Wait, sorry for the spam. The test continue to failing in GH. Let me try one more thing related to the test setup.
😃

@pepoviola
Copy link
Contributor Author

ping @evoxmusic / @clarity0 now is ready to review and merge :). Thanks!

Copy link
Contributor

@evoxmusic evoxmusic left a comment

Choose a reason for hiding this comment

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

look at my last comment, then we are all good 🥳

@evoxmusic evoxmusic merged commit 28003e7 into Qovery:main May 19, 2021
@evoxmusic
Copy link
Contributor

Thank you and congrats for your first accepted PR @pepoviola 🔥

@pepoviola pepoviola mentioned this pull request May 19, 2021
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.

None yet

2 participants