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

Merged
merged 5 commits into from Mar 5, 2018

Conversation

Projects
None yet
3 participants
@Geoffrey-A
Contributor

Geoffrey-A commented Jan 10, 2018

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();

Geoffrey-A added some commits Jan 9, 2018

Fix RedisHash.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
Fix RedisZSet.remove().
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

This comment has been minimized.

Member

s-ludwig commented Jan 13, 2018

Thanks! LGTM.

@s-ludwig s-ludwig added the auto-merge label Jan 13, 2018

@dlang-bot dlang-bot removed the auto-merge label Jan 16, 2018

@Geoffrey-A

This comment has been minimized.

Contributor

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 from Fix: RedisValue.remove() hidden in RedisZSet and RedisHash to Fix: forward RedisValue methods hidden in redis types by overloads Feb 4, 2018

@s-ludwig

This comment has been minimized.

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

3 of 4 checks passed

codecov/project 35.069% (-23.39%) compared to 07a47ea
Details
codecov/patch Coverage not affected when comparing 07a47ea...d2ca321
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment