apple / swift-http-types

Version-independent HTTP currency types for Swift
Apache License 2.0
894 stars 47 forks source link

Make `HTTPResponse.Status` conform to `Comparable` #60

Closed noremac closed 2 months ago

noremac commented 2 months ago

Conforming HTTPResponse.Status to Comparable makes it possible to create ranges of status codes. It's easy enough to drop down and check .code, but sometimes it's nice to stay within the domain of HTTPResponse.Status.

guoye-zhang commented 2 months ago

Thanks for opening the PR! I'm interested in understanding your use case. Status has a property kind which represents the class of the status (e.g. status.kind == .successful). Do you have a use case for forming a range beyond what kind already supports today?

Status is an extensible enum with an underlying value. Are there examples of other similar enums which support Comparable?

noremac commented 2 months ago

Thanks for opening the PR! I'm interested in understanding your use case. Status has a property kind which represents the class of the status (e.g. status.kind == .successful). Do you have a use case for forming a range beyond what kind already supports today?

Status is an extensible enum with an underlying value. Are there examples of other similar enums which support Comparable?

Hello, here's my use case FWIW, and I'm happy to close this PR if the motivation is not interesting enough! Thanks for the consideration.

I've got a library built on top of/alongside HTTPTypes that by default treats non 200s as errors and throws something like:

public struct UnacceptedHTTPStatus: Error, Sendable {
  public let data: Data
  public let response: HTTPResponse
}

and I've then defined operators like this:

public func ~= (lhs: HTTPResponse.Status, rhs: some Error) -> Bool {
  if let error = rhs as? UnacceptedHTTPStatus {
    error.response.status == lhs
  } else {
    false
  }
}

public func ~= (lhs: some RangeExpression<HTTPResponse.Status>, rhs: some Error) -> Bool {
  if let error = rhs as? UnacceptedHTTPStatus {
    lhs.contains(error.response.status)
  } else {
    false
  }
}

public extension RangeExpression where Self == Range<HTTPResponse.Status> {
  static var clientError: Self {
    400..<500
  }

  // and the rest as well
}

which then lets me do:

do {
  _ = try await foo()
  // happy path log
} catch HTTPResponse.Status.notModified {
  // info level log
} catch HTTPResponse.Status.clientError {
  // a more serious error log
}

I could of course always catch UnacceptedHTTPStatus directly and check kind in there but I've found myself catching a few specific statuses or ranges frequently enough that introducing the operator cleaned things up a bit.

guoye-zhang commented 2 months ago

I tried this but it didn't work. Wondering why there is a distinction between structs and enums.

public func ~= (lhs: HTTPResponse.Status.Kind, rhs: some Error) -> Bool {
  if let error = rhs as? UnacceptedHTTPStatus {
    error.response.status.kind == lhs
  } else {
    false
  }
}

Still not completely sold that status needs to be comparable. E.g. .ok < .notFound doesn't make much sense. Is this a conventional usage of pattern matching? Can you make UnacceptedHTTPStatus comparable instead?

Lukasa commented 2 months ago

For what it's worth I agree with @guoye-zhang 's suggestion here: the better move is almost certainly to make UnacceptedHTTPStatus Comparable.

noremac commented 2 months ago

I think y'all are right! Agreed that something like .ok < .notFound doesn't make much sense, and some of my range use cases are better served by existing things like kind (or, better served by just generally being built differently!)

Thanks for taking a look.