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

Fix: forward RedisValue methods hidden in redis types by overloads #2026

Conversation

Geoffrey-A
Copy link
Contributor

All supported redis values now work as per the test below:

auto db = connectRedis("127.0.0.1").getDatabase(0);
db.getAsList("test_list").remove();
db.getAsSet("test_set").remove();
db.getAsZSet("test_zset").remove();
db.getAsHash("test_hash").remove();
db.getAsString("test_string").remove();

The snipped `m_db.getAsHash("test_hash").remove();` caused the following error at runtime:
object.Exception@../../.dub/packages/vibe-d-0.8.2/vibe-d/redis/vibe/db/redis/redis.d(1370): ERR wrong number of arguments for 'hdel' command
The snipped `m_db.getAsZSet("test_zset").remove();` caused the following error at runtime:
object.Exception@../../.dub/packages/vibe-d-0.8.2/vibe-d/redis/vibe/db/redis/redis.d(1370): ERR wrong number of arguments for 'zrem' command
@s-ludwig
Copy link
Member

Thanks! LGTM.

@Geoffrey-A
Copy link
Contributor Author

Geoffrey-A commented Feb 4, 2018

Seeing it was not merged after a few days, I added a few commits. They fix the forwarding of the hidden RedisValue methods in the redis types which were hiding them:

  1. forward the boolean result returned by remove();
  2. forward exists() in RedisHash, which was hiding it with its own overload.

I also checked there was no other method from RedisValue that was hidden by any redis type.

Since I committed, the dlang bot removed the auto-merge tag.

Please allow me a few questions:
What is the benefit of the auto-merge tag over simply merging?
Why was not this PR merged? I noticed there was at that time some CI build failures, maybe that was the reason? However the failures seemed completely unrelated to this PR.

@Geoffrey-A Geoffrey-A changed the title Fix: RedisValue.remove() hidden in RedisZSet and RedisHash Fix: forward RedisValue methods hidden in redis types by overloads Feb 4, 2018
@s-ludwig
Copy link
Member

s-ludwig commented Mar 5, 2018

The "auto-merge" is supposed to provide a "shoot and forget" mode, where you don't have to wait until all CI checks complete. Unfortunately there are currently still too many network related failures, so that this often fails and then sits in the queue unnoticed. But this will hopefully be sorted out in a while.

Fortunately I can merge immediately now, since the tests have already finished...

@s-ludwig s-ludwig merged commit 30dbde7 into vibe-d:master Mar 5, 2018
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