Kitura / Kitura-redis

Swift Redis library
Apache License 2.0
95 stars 26 forks source link

Added additional Redis functions #12

Closed chiahuang closed 8 years ago

chiahuang commented 8 years ago

Description

Added additional Redis functions

Motivation and Context

Need these additional Redis functions to use them for another project

How Has This Been Tested?

Did unit testing on the additional Redis functions

Checklist:

rfdickerson commented 8 years ago

We have identified 6 test cases (not related to the new functions this PR addresses) that are failing. We will work on helping to get those fixed eventually once we have finished our todolist-redis implementation.

shmuelk commented 8 years ago

@chiahuang Overall it looks fairly good. A couple of comments:

  1. Please undo your changes to Tests/SwiftRedis/CommonUtils.swift. The test code reads in from files the host and the password on purpose.
  2. For all of your APIs where there is an array, also please have version that take a list of that type (i.e. if the parameter is [String], there should be a second function that takes String... . The later should call he former. Look at the API for mset as an example.
  3. You didn't update Tests/LinuxMain.swift. Did you run your tests on Linux?
rfdickerson commented 8 years ago

Thanks for the feedback, @shmuelk! Those will be mostly simple changes. However, we were having trouble getting XCode to run the test cases (with a relative path password.txt file). How can we get XCode set up to find the password.txt file when we run the tests in the IDE?

@chiahuang, we should probably set you up with a Vagrant environment to test whether it works in Linux.

shmuelk commented 8 years ago

@rfdickerson We do not run tests in XCode. All tests are run by using swift build and swift test. You can also do a make test from within the Kitura-redis repository.

shmuelk commented 8 years ago

@chiahuang I guess I didn't explain myself quite so well in my previous comment.

What I was looking for in the style of the APIs is something as follows, using zrem as an example:

public func zrem(_ key: String, members: String..., callback: (Int?, error: NSError?) -> Void) { zremArrayOfMembers(key, members: members, callback: callback) }

public func zremArrayOfMembers(_ key: String, members: [String], callback: (Int?, error: NSError?) -> Void) { ... }

The idea is that there are a pair of functions, one that uses ellipses and one that uses an array. The function that uses he ellipses simply calls the one that uses the array. I would name them the same, but I have seen in the past compiler crashes in such cases.

chiahuang commented 8 years ago

@shmuelk Thanks, I have conform to the API style. If there is any problems, let me know.

shmuelk commented 8 years ago

@chiahuang The APIs now look great.

I have only one small comment on the tests. In the file Tests/SwiftRedis/TestSetCommands.swift on line 38, please remove authenticate: false. Your code passes the tests only because a previous test case authenticate with the Redis server.

shmuelk commented 8 years ago

@chiahuang Thanks for fixing the tests.

But Why the dependency on Kitura-net? Please put the dependency back to Kitura-sys with a minor of 21.

rfdickerson commented 8 years ago

Ah, so changing the Kitura-sys Package to use minor 21 will fix the problem? I believe we were using Kitura-sys 0.17 before and we were getting a dependency version conflict on the 'Kitura' package. We will make that change.

shmuelk commented 8 years ago

@chiahuang The correct minor for Kitura-sys is 21. If you are using this version of Kitura-redis, you should also be using Kitura with a minor of 21.

I'm going to merge this PR and then fix the Package.swift file