Make parsing initializers throw errors on failure

[czechboy0]

Currently some of them assert, some of them fail more gracefully. This needs to be unified with the new Swift 2 error handling system.

[cojoj]

I think this one should be a more global approach and try to replace any NSError return pointer be replaced by throws.

I suggest changing this while writing tests but we need to find a way how to test throwing methods to fully cover them... My idea is to write two tests per throwing method - in first test correct path and in the latter the path which throws exception.

We should create new ErrorType enum which will resemble what happened and probably these methods will be optional or will return optionals.

[cojoj]

@czechboy0 I think this one should be marked as a blocker right now because whatever we'll try to implement we'll be basing on exisinting XcodeServerEntity classes.

[czechboy0]

In light of this, I suggest we slow down on migrating the JSON parsing initializers to throw, because the code will become pretty ugly. I think we should focus on other issues before they fix it and we'll be able to do it the way it should have been all along. What do you think, @cojoj and @esttorhe?

[esttorhe]

I totally agree. The code looks cluttered and FUGLY as hell; would rather keep it «failable» but clean.

[czechboy0]

This issue is now blocked, we're waiting for Swift to become sane about throwing out of initializers without initializing all properties.

[esttorhe]

@czechboy0 perhaps remove the up-for-grabs until this is «unblocked»?

[czechboy0]

Good point 👍

[pmkowal]

I know that we are waiting for some Swift changes now, but when I run the test below with the "//\\https://127.0.0.1" value as a host parameter I receive EXC_BAD_ACCESS.

    func testDictionaryInit() {
        XCTempAssertNoThrowError("Failed to initialize the server configuration") {
            let json = [
                "host": "//\\https://127.0.0.1",
                "user": "ICanCreateBots",
                "password": "superSecr3t"
            ]

            let config = try XcodeServerConfig(json: json)

            XCTAssertEqual(config!.host, "https://127.0.0.1", "Should create proper host address")
            XCTAssertEqual(config!.user!, "ICanCreateBots")
            XCTAssertEqual(config!.password!, "superSecr3t")
        }
    }

Maybe in this case we could set JSONSerializable init method as non failable, like below, what do you think?

public protocol JSONSerializable {
    init(json: NSDictionary) throws
    func jsonify() -> NSDictionary
}

[czechboy0]

Well, we still need a way to not return an object if the JSON is corrupt or we don't have all required fields. Since throwing is still highly impractical (because you have to initialize all variables before throwing, thus not allowing for them to be lets), having the initializer failable makes it possible for us to have this behavior. Hopefully Apple will fix this, so that you can throw at any point in initializers, so we'll be able to get rid of the return optional paradigm.

Btw, why does the host have two forward and two back slashes?

[pmkowal]

Btw, why does the host have two forward and two back slashes?

I wanted to write some tests and I was digging through the public required convenience init?(json: NSDictionary) throws method of XcodeServerConfig class.
I noticed that when you pass the host parameter with two forward and two back slashes to designated init it throws an error, but when you pass it to convenience init you get EXC_BAD_ACCESS, which shouldn't occur IMO.

[cojoj]

@pmkowal any idea which line crashes? Have you tried setting breakpoints? I wonder which line is this bad boy... 😈

[pmkowal]

The problem is when the public required init(var host: String, user: String?=nil, password: String?=nil) throws method throws ConfigurationErrors.InvalidHostProvided(host) exception in line 84.
When I catch this exception in line 129 and return nil instead of throwing an error it behaves good (no EXC_BAD_ACCESS). Also when I was trying to pass the malformed host directly to public required init(var host: String, user: String?=nil, password: String?=nil) throws method, everything behaves as it should.

[cojoj]

maybe because this:

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

should become this:

do {
    try self.init(host: host, user: json.optionalStringForKey("user"), password: json.optionalStringForKey("password"))
} catch {
    Log.error("Couldn't initialize server configuration: \(error)")
}

❓

[cojoj]

Me and @pmkowal sat for a while on this and discussed possibilities... Seems like we're mixing too much... Okay, it's supported to call non-failable init from inside of failable init (The Swift Programming Language) - fine. But we're messing throws with this and I guess it's a 💥 thing...

Here are my solutions:

• Comment out convenince init?
• Make convenience init non-failable

[czechboy0]

Is it just me or does this work fine in structs, but doesn't in classes? Seems like we can in fact throw before initializing all stored properties in a struct.

[cojoj]

@czechboy0 it seems very likely... I will try it (😬) ASAP and let you know!

[cojoj]

Yeah, it work for structs, but I'm also almost sure that this was always possible for structs... ¯_(ツ)_/¯

It looks like a lot of work with converting everything to structs right now... Also, consider that we'll need some protocol Entity with extensions, so we can provide default implementation for some properties.
But... YEAH, I'M IN FAVOR OF DOING THIS! Crusty would be happy!