stormpath / Turnstile

An authentication framework for Swift.
Apache License 2.0
165 stars 29 forks source link

Allow create session and destroy session to throw. #20

Closed hhanesand closed 2 weeks ago

hhanesand commented 7 years ago

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.

tanner0101 commented 7 years ago

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?

edjiang commented 7 years ago

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

hhanesand commented 7 years ago

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?

edjiang commented 7 years ago

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 =]

hhanesand commented 7 years ago

You got it 👍

edjiang commented 7 years ago

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.