Closed rafalwojcik closed 2 years ago
@JoeMatt agree there should be some error handling.
Do you think about: completion: ((success: Bool) -> Void)? = nil
or completion: ((Error?) -> Void)? = nil
?
completion: ((success: Bool) -> Void)? = nil
Would you mind if I suggest one more variant?
public enum ReplaceRootViewError: Error { // smth like that not sure about naming
... // describe all your failure cases
}
public enum Status {
case success
case failure(ReplaceRootViewError) // or its can be just Error
}
func replaceViewController(with next: UIViewController, completion: ((status: Status) -> Void)? = nil) {
...
// failure
completion?(.failure(ReplaceRootViewError.someError))
...
// success
completion?(.success)
...
}
It would be Ideal to throw error as @JoeMatt said, to identify what was wrong. You need only give descriptive names to these errors (the hardest part 🥲)
Also the last case with window should also call completion when transition finished (now its not). And fix cases that just make return and telling nothing to function caller about completion (so caller that waits for completion never find out that function completed).
Hope, it was helpful :)
Hope, it was helpful :)
This looks really helpful indeed, I'll take a look at incorporating as soon as I have the bandwidth or if someone else opens/updates a PR for review.
Thanks @dimatarelkin
@dimatarelkin, if you're thinking of using an enum to model the completion status, why not use Swift's Result?
public enum ReplaceRootViewError: Error {
...
}
typealias CompletionResult = Result<Void, ReplaceRootViewError> // Could also use a plain Error here
func replaceViewController(with next: UIViewController, completion: ((result: CompletionResult) -> Void)? = nil) {
...
// failure
completion?(.failure(.someError))
...
// success
completion?(.success(()))
...
}
@dimatarelkin, if you're thinking of using an enum to model the completion status, why not use Swift's Result?
public enum ReplaceRootViewError: Error { ... } typealias CompletionResult = Result<Void, ReplaceRootViewError> // Could also use a plain Error here func replaceViewController(with next: UIViewController, completion: ((result: CompletionResult) -> Void)? = nil) { ... // failure completion?(.failure(.someError)) ... // success completion?(.success(())) ... }
Yeah, that would be nice, thanks @oconnelltoby
Fix issue from ticket: #717