Improved fix for issue #13. Removed KitruaSys dependency.

dsperling commented 8 years ago

Description

This improves the readability of fix #13 as well as removes the dependency of Kitura-sys. Note, Kitura-sys is still required for the unit tests and therefore remains in Package.swift. Since only the unit tests now use Kitura-sys, I removed the requirement of matching the minor version (which should help Kitura-redis be more forwards compatible).

Motivation and Context

The motivation of removing Kitura-sys dependencies was that updates to Kitura and Kitura-sys were breaking projects that used Kitura-redis, as Kitura-redis was specifying a different Kitura-sys dependency.
Readability was improved and the use of Optional unwrapping eliminated.

The following changes were made:

Using RedisString("...").asData as the main function to create a safe NSData from a string.
Moved the ?? NSData() fix for Linux to RedisMisc.swift
Removed all warnings in the unit tests from 0606 Toolchain
Made Kitura-net dependency only specify majorVersion: 0
Added testing notes using Docker and redis:alpine offical image
Added readme with sample code
Other small readme tweaks
How Has This Been Tested?

Unit tested on Mac and Linux

Checklist:

[x] I have submitted a CLA form
[x] If applicable, I have updated the documentation accordingly.
[ ] If applicable, I have added tests to cover my changes.

shmuelk commented 8 years ago

@dsperling Thanks for your continued interest in Kitura-redis.

A couple of questions:

If you indeed removed the dependency on KituraSys, as it appears in most of the files, why did you leave it in the Package.swift file?
I think having a dependency on majorVersion of 0 of KitraSys is very bad. I understand that it makes migration easier, but it's a bad idea.
As far as the warnings go on the tests go, I think it would be better to add the @discardableResult annotation to the transactional APIs in RedisMulti.swift

dsperling commented 8 years ago

@shmuelk Comments:

TestListPart3.swift uses the Queue class from Kitura-sys. Queue then depends on SysUtils.doOnce. I have seen in some other Kitura threads that the intention is to get rid of Kitura-sys at some point. Is this still the plan?
OK, understood. I would like to remove Kitura-sys altogether if you agree. We could inline the Queue code or add those two files from Kitura-sys to Tests/SwiftRedis.
Yes, that is a much better solution.

shmuelk commented 8 years ago

@dsperling I understand things better now. I even remember why I added that queue in the test code.

I would prefer at this time to not remove Kitura-sys as a dependency. The goal is once libdispatch on Linux gets the same nice object oriented overlay that GCD has on Darwin, to start removing our dependency on Kitura-sys. At this time please have an appropriate minor in the dependency. Also update the README.md as appropriate.

dsperling commented 8 years ago

@shmuelk

I looked into what it would take to remove Kitura-sys. If the following code was added to TestListPart3.swift we could remove Kitura-sys now. Let me know which direction you want to take and will update the PR later today.

import Dispatch

public class Queue {

    internal let osQueue: dispatch_queue_t

    public init(label: String?=nil) {
        osQueue = dispatch_queue_create(label != nil ? label! : "",
                                        DISPATCH_QUEUE_CONCURRENT)
    }

    public func enqueueAsynchronously(_ block: () -> Void) {
        dispatch_async(osQueue, block)
    }
}

ianpartridge commented 8 years ago

+1 to removing this dependency on Kitura-sys as it seems minimal extra code. Plus once the new Swift 3 GCD overlay is available we can even remove that.

dsperling commented 8 years ago

@shmuelk I made the changes you requested using @discardableResult and reverted all the changes previously made in the unit tests. I also moved the Queue class, removing all Kitura-sys dependencies. To make the transition simpler when the DispatchQueue class becomes available, I aligned the Queue API to match DispatchQueue.

shmuelk commented 8 years ago

@dsperling The problem I have with in-lining the queue class is that when we migrate to the 06/20 drop of Swift, Darwin gets the new GCD overlay while Linux doesn't. I'd rather have to deal with that in only one place rather than in many.

ianpartridge commented 8 years ago

Maybe we should wait until the new GCD overlay is available on Linux, before moving up snapshots.

dsperling commented 8 years ago

Sounds good. I will keep this PR open and keep it updated for future snapshots.

ianpartridge commented 8 years ago

Personally I think we should merge it now, but it's @shmuelk 's decision 😄

shmuelk commented 8 years ago

@dsperling I really like this PR. You really have done good things here. I don't like the idea of keeping the PR open until we can replace the usage here of Kitura-sys with a better overlay for Dispatch on Linux. I have no idea how long that will take and it will be a shame to keep this PR up to date just for that.

I'm going to merge it and that add back the dependency on Kitura-sys and remove your added queueing code and use the code from Kitura-sys. There are also now merge conflicts, apparently due to the other PR that I just merged. I'll take care of those as well.

Kitura / Kitura-redis