Turnstile icon indicating copy to clipboard operation
Turnstile copied to clipboard

Allow create session and destroy session to throw.

Open hhanesand opened this issue 9 years ago • 6 comments

Took the liberty of adding 'throws' to both destroySession and createSession. If you disagree with this, feel free to close and do whatever you think is best.

FYI, my use case for idestroySession' throwing is because the call to the database to delete the session could fail and I would like to communicate this to the callee of the API.

hhanesand avatar Nov 15 '16 04:11 hhanesand

This looks good to me, but we'd need some way to offer backward compatibility if we don't want to bump this up a major version.

Perhaps we could provide deprecated protocol extensions that force try?

tanner0101 avatar Nov 16 '16 17:11 tanner0101

Yeah, that sounds like a good approach to this =]

edjiang avatar Nov 16 '16 18:11 edjiang

I am very new to keeping up with backwards compatibility when it comes to APIs and I need some help with this one. I tried creating a protocol extension as follows :

public extension SessionManager {

    @available(*, unavailable, message: "Use throwing version.")
    public func createSession(account: Account) -> String {
        return try! createSession(account: account)
    }

    @available(*, unavailable, message: "Use throwing version.")
    public func destroySession(identifier: String) {
        return try! destroySession(identifier: identifier)
    }
}

but it doesn't work, as the syntax at the calling sites still needs to be changed....

What am I doing wrong here, or what is the missing piece?

hhanesand avatar Nov 16 '16 23:11 hhanesand

I think Tanner meant something more along the likes of:

public extension SessionManager {
    public func createSession(account: Account) -> String {
        return try? createSession(account: account) ?? ""
    }
}

While returning an empty string is not quite the indended result, that's the only way that this could fail in the current implementation, so I think it works out.

If you want to try that, and update the tests, this would be awesome! Otherwise I'll get on this =]

edjiang avatar Nov 18 '16 01:11 edjiang

You got it 👍

hhanesand avatar Nov 18 '16 01:11 hhanesand

Awesome, thanks @hhanesand! Also, don't worry if Travis complains about the Facebook test -- that one will always fail if it's done from external repos.

edjiang avatar Nov 18 '16 01:11 edjiang