some refactoring

[tomerd]

motivation: better code structure

changes:

• abstract errors as enum instead of nested structs
• make configuration a nested class

[tomerd]

addresses #5

[artemredkin]

I'm happy with this change (it was implemented like this in the beginning), but @Lukasa suggested to use nested structs instead.

[tomerd]

oh okay, i personally prefer the enum but maybe missing something? @Lukasa is that a matter of taste or is there a strong technical reason?

[Lukasa]

There is a strong technical reason: enums are non-extensible.

Adding a case to an enum is a semver major change, as switch statements need to be exhaustive. This means that adding any new errors requires adding a new error enum each time you do it, leading to an explosion of error enums which makes it hard to find all the errors the library can throw, and reducing the very real advantage that error enums have over error structs.

There are two ways out of this:

1. Using nested structs. This makes it easy to find all the errors the program can throw, and allows users to have some idea of what the exhaustive list is at any one time, but the ergonomics aren't ideal.
2. Hacking together an open enum using a struct and static lets. You still end up with an error struct instead of an error enum, but it seems to behave a bit more like an error enum.

Either approach works.

[weissi]

I agree with @Lukasa and think we should go for (2). Until we get open enums, I think we should fabricate them ourselves like this:

import Foundation

public struct SandwichError: Error, Equatable, CustomStringConvertible {
    private enum SandwichErrorCode: Int {
        case tooLittleSalami = 1
        case tooLittleMustard
        case noTomatoes
        case noBread
    }

    private var code: SandwichErrorCode

    private init(code: SandwichErrorCode) {
        self.code = code
    }

    public var description: String {
        return "SandwichError.\(String(describing: self.code))"
    }

    public static let tooLittleSalami: SandwichError = .init(code: .tooLittleSalami)
    public static let tooLittleMustard: SandwichError = .init(code: .tooLittleMustard)
    public static let noTomatoes: SandwichError = .init(code: .noTomatoes)
    public static let noBread: SandwichError = .init(code: .noBread)
}

func inspectErrorWithSwitch(_ error: Error) {
    if let error = error as? SandwichError {
        switch error {
        case .tooLittleSalami:
            print("NEED MORE SALAMI!")
        case .tooLittleMustard:
            print("NEED MORE MUSTARD!")
        case .noTomatoes:
            print("Can I please get a tomato?")
        default:
            print("ooh, some error I didn't know existed: \(error)")
        }
    } else {
        print("totally unexpected error")
    }
}

func inspectErrorWithCatch(_ e: Error) {
    do {
        throw e
    } catch let error as SandwichError where error == .tooLittleSalami {
        print("NEED MORE SALAMI!")
    } catch let error as SandwichError where error == .tooLittleMustard {
        print("NEED MORE MUSTARD!")
    } catch let error as SandwichError where error == .noTomatoes {
        print("Can I please get a tomato?")
    } catch let error as SandwichError {
        print("ooh, some error I didn't know existed: \(error)")
    } catch {
        print("totally unexpected error")
    }
}

inspectErrorWithSwitch(SandwichError.tooLittleSalami)
inspectErrorWithSwitch(SandwichError.tooLittleMustard)
inspectErrorWithSwitch(SandwichError.noTomatoes)
inspectErrorWithSwitch(SandwichError.noBread)
inspectErrorWithSwitch(NSError())

inspectErrorWithCatch(SandwichError.tooLittleSalami)
inspectErrorWithCatch(SandwichError.tooLittleMustard)
inspectErrorWithCatch(SandwichError.noTomatoes)
inspectErrorWithCatch(SandwichError.noBread)
inspectErrorWithCatch(NSError())

That works almost as nice as closed enums but we can add cases

[weissi]

also the print out of unknown error cases is nice:

ooh, some error I didn't know existed: SandwichError.noBread

I think new Swift libraries should construct errors as above. NIO doesn't do it (yet) because we hadn't figured that out before writing NIO ;)

[ianpartridge]

Yeah we used a similar pattern in Kitura's RequestError: https://github.com/IBM-Swift/KituraContracts/blob/f56a8e45c9ec2de46c3da3e0e719fc1c9653c274/Sources/KituraContracts/Contracts.swift#L38

I think it's the least-worst option until @jrose-apple gives us extensible enums :)

[weissi]

@ianpartridge but in RequestError you expose the underlying code which for 'request error' is probably what you want. The scheme I outlined above doesn't expose at all there's an underlying code which I like (unless it's well-known error codes like HTTP ones).

[ianpartridge]

Yes, similar pattern, not identical. It ticks the extensibility box which is the big problem with public enum (and makes them almost an anti-pattern in libraries).

[tomerd]

cool 😎 i’ll rework this

[tomerd]

c1a1ee6

[tanner0101]

+1. What about nesting HTTPRequest and HTTPResponse, too? HTTPClient.Request / HTTPClient.Response.

[ianpartridge]

I think making the errors a struct with static let is the best approach.

[weissi]

because this doesn't have a public API yet, I'd like to refactor the errors to an extensible pattern instead.

[tomerd]

+1. What about nesting HTTPRequest and HTTPResponse, too? HTTPClient.Request / HTTPClient.Response.

69d9f2b

[weissi]

it still says SandwichError :D

[weissi]

I'm happy as soon as the 'SandwichError' printout is renamed to the real error name D: