Agreed.

from xcodeserversdk.

I also think there is a slightly larger issue in many places of the framework. When I was writing the first lines of this code back in December '14, I had no idea what the best practices in Swift were, so I was treating a lot of the parsing as a "parse well or crash" situation. Now, however, I think we should turn all of the parsing initializers into init? failable initializers, so that we can always fail gracefully. This will require some changes in all the entities.

Plus, as you said, not all the initializers verify the input data in the same way, so this is more of an open question how to do that well everywhere without too much code duplication.

from xcodeserversdk.

Actually, instead of using failable initializers, we should use init(...) throws, so that we signal that the object should either parse correctly or fail to initialize altogether. I started that work already.

from xcodeserversdk.

Exactly, we should provide a graceful way of informing developer that something may fail and he/she has to handle this.

from xcodeserversdk.

Sorry to step in here; haven't had the time to fully check the current implementation of the API but why not «convert» the hosts to an enum that contains the scheme as a case ??

Something like:

public enum XcodeHost {
  case HTTP(String)
  case HTTPS(String)
}

Or even have cases with the username and/or password?

Doing it like this will eliminate the need to add a «valida host» method or logic.

Just throwing some ideas, perhaps there's a better approach though ¯_(ツ)_/¯

from xcodeserversdk.

Even though, we will need some sort of checking whether enum's cases are correct, right?

In case of XcodeServer we'll probably skip HTTP support as it operates on HTTPS and in that case a simple guard statement can handle our case.

from xcodeserversdk.

it depends on how we structure the enum; it could be made in a way that «safe guards» it to a point where receiving a case of said enum is «valid» no matter what.

But yeah; I agree that a simple guard would work to check the use of https scheme

from xcodeserversdk.

If you have some idea, feel free to PR 😉 I think that enum scenario looks promising and Swifty!

from xcodeserversdk.

I was playing a little bit with this; probably abandoning the idea of the enum because after reading the code with more detail I don't think it will add much value to the current implementation.

I toyed with the idea of substituting the init checks with throws but found an issue; the JSONSerializable expects a different signature (if I add the throws in there changes completely and compiler chokes).

Was planning on replacing it with something like this:

public enum ConfigurationErrors : ErrorType {
    case NoHostProvided
    case InvalidHostProvided(String)
    case InvalidSchemeProvided(String)
}

public class XcodeServerConfig : JSONSerializable {
    public required init(var host: String, user: String?=nil, password: String?=nil) throws {
        guard let url = NSURL(string: host) else {
            throw ConfigurationErrors.InvalidHostProvided(host)
        }

        guard url.scheme.isEmpty || url.scheme == "https" else {
            let errMsg = "Xcode Server generally uses https, please double check your hostname"
            Log.error(errMsg)

            throw ConfigurationErrors.InvalidSchemeProvided(errMsg)
        }

        // validate if host is a valid URL
        if url.scheme.isEmpty {
            // exted host with https scheme
            host = "https://" + host
        }

        self.host = host
        self.user = user
        self.password = password
        self.availabilityState = .Unchecked
    }

    public required convenience init?(json: NSDictionary) throws {
        guard let host = json.optionalStringForKey("host") else {
            throw ConfigurationErrors.NoHostProvided
        }

        try self.init(host: host, user: json.optionalStringForKey("user"), password: json.optionalStringForKey("password"))
    }
}

¯_(ツ)_/¯

Maybe change the protocol but I don't know if you guys are up to it

from xcodeserversdk.

IMHO, throws version is the one we want here and with this approach we're taking Swift 2 path, which seems to be righteous one at the moment 😉 Everything is up to @czechboy0, he's the master here!

from xcodeserversdk.

I also added default values for username and password; that way consumers of the API could call the config init with only the host (since its the only required parameter ATM)

from xcodeserversdk.

Woah, you sure you don't have to provide account credentials to call integration on XcodeServer? 😮

from xcodeserversdk.

TBH I haven't played that much with the API; I'm basing this in the fact that the configuration class accepts username and password as Optionals which makes me think they can be nil.

If I'm understanding the API wrong then it should be changed to not accept Optionals and instead force the passing of values that will be consumed later.

from xcodeserversdk.

I don't know the purpose of marking them as Optionals but I'm almost sure when you dig the Xcode API you'll see that every request requires something marked as auth.verifyIfServiceIsAllowed...

from xcodeserversdk.

ConfigurationErrors

Thanks, @esttorhe I actually really like what you did up there with the ConfigurationErrors and the whole throwing initalization logic. I think that's the right way to go (would love a PR with this).

When I was playing with migrating some other initializers to also throw, I was getting a compiler error complaining about me not initializing all stored properties before throwing (meaning you can't nicely parse and conditionally fail+throw on every field before you assign each some value). Have you seen this?

Username and password

The fields are indeed optional, because you can setup your Xcode Server to allow anyone to view/create bots, in which case any values for username+password are going to be valid. In case you don't provide the necessary credentials and call an API that needs it, it will respond with a classic 401 Unauthorized response. Classic REST.

from xcodeserversdk.

@czechboy0 I just encountered the same compiler complain about not initializing all properties before throwing; which IMHO should be radar worthy.

There's no point in having to fully initialize the properties of an init that's throwing an error :/

I had to parse my code like this:

guard let url = NSURL(string: host) else {
    /// Had to be added to silence the compiler ¯\_(ツ)_/¯
    self.host = ""; self.user = nil; self.password = nil

    throw ConfigurationErrors.InvalidHostProvided(host)
}

guard url.scheme.isEmpty || url.scheme == "https" else {
    let errMsg = "Xcode Server generally uses https, please double check your hostname"
    Log.error(errMsg)

    /// Had to be added to silence the compiler ¯\_(ツ)_/¯
    self.host = ""; self.user = nil; self.password = nil

    throw ConfigurationErrors.InvalidSchemeProvided(errMsg)
}

And I don't like the look of it one little bit.
I might try to code it with a workaround because that looks like code smell 💩

I'll file a radar regarding this and will post a link to it in case you guys want to dupe it.

from xcodeserversdk.

http://openradar.me/21514477

Update:

@esttorhe It's an implementation limitation. Cleaning up partially initialized classes is tricky.

— Joe Groff (@jckarter) June 23, 2015

from xcodeserversdk.

Thanks, @esttorhe, great work!

from xcodeserversdk.