StatusCode enum

[seanmonstar]

There's a couple of questions, maybe they should be broken out into separate issues, but I'll start by listing them here:

1. Should it even be an enum? There is the StatusCode::Unregistered(u16) variant, since really any number between 100 and 599 is a valid status code. The reason for it being an enum originally in hyper is to take advantage of match statements. if let StatusCode::Created = status reads better than if status == 201 in my opinion. The problem with is being an enum is that it's a breaking change any time a new status code is defined, and we want to name it.

   I had considered for a bit having a newtype around u16, but the associated constants feature is required to make it complete. It could look like:

   pub struct StatusCode(u16);
   
   impl StatusCode {
       pub fn new(code: u16) -> StatusCode;
   
       pub const Ok: StatusCode = StatusCode(200);
   }

2. Does the StatusClass pull its own weight? The various specs declare that the first digit of the code determines its category or class. hyper has a StatusClass enum that allows any StatusCode to be check for its class. As mentioned in RFC7321, if a client doesn't recognize a code, it should still be able to recognize its class, such as 471 being treated as a client error, like 400.

3. Should converting from a primitive depend on the unstable TryFrom feature? Since the specs say that a client should always be able to determine a status class from a code, and there are only classes defined in the specs for 1xx, 2xx, 3xx, 4xx, and 5xx, it can be implied that any other status code is invalid or illegal. Does it make sense then that converting from a u16 to a StatusCode is a fallible action? If so, the best case to do that is probably TryFrom, which is currently unstable, but looks super close to stabilization.

[alexcrichton]

I think I like the idea of using an opaque struct, perhaps with convenient constructors like StatusCode::ok() and code.is_ok() to check it. Maybe the class methods could be folded into that as well?

[carllerche]

I tend to prefer opaque structs as well, though an enum has advantages here.

I'm leaning towards an opaque struct. It has the advantage of being able to be forwards compatible. Helper fns can be added as @alexcrichton pointed out.

One option to consider to improve the readability is to add constants in the status module:

if status == status:: Created {
    // ...
}

Re: StatusClass. I would say to be conservative for now. We can always add later. So, my vote would be to not include it for now.

Re: TryFrom, I will punt to the others :)

[seanmonstar]

Yea, I've been leaning towards an opaque struct as well. Perhaps with each current variant translated into a constructor function, like StatusCode::created().

It also makes sense to ditch StatusClass, and just have the status checking methods on StatusCode.

The only thing lost is ergonomic matches. The left side of a match arm can't use functions, and when using constants, it's far easier to mistype a constant name and actually have just a renamed catch pattern.

With the idea of using associated constants, users could probably still match on something like StatusCode::Badrequest, and Rust would notice that refers to nothing.

[alexcrichton]

I personally feel like that's an acceptable tradeoff. The purpose here is to provide a maximally implementation flexible (and extensible) type into the future, not to be the most ergonomic to match on (which is also the most restrictive from an implementor's point of view)

[seanmonstar]

I have a working example of much of this in: https://github.com/seanmonstar/http-rs/blob/6059d2ba8633d9478ea173718b155731b55df07a/src/status.rs

Shortened gist is:

pub struct StatusCode(u16);

impl StatusCode {
    // ... constructors for every registered status
    pub fn ok() -> StatusCode {
        StatusCode(200)
    }

    // ... and some methods
    pub fn is_success(&self) -> bool {
        300 > self.0 && self.0 >= 200
    }
}

There was 1 conflict when defined the 100 Continue code with a Rust keyword, so it became StatusCode::continue_().

[alexcrichton]

I'd also be fine with something like StatusCode::kontinue

[carllerche]

I also don't think there needs to be function constructors for all status codes, maybe only the common ones. You can always construct it w/ the actual number:

StatusCode::new(100) isn't terrible.

[seanmonstar]

I personally find the constructor names more beneficial for obscure status codes. I prefer StatusCode::too_many_requests() to StatusCode::new(429), for instance.

Follow up question is if there should be a new constructor that is infallible. If there is, then it's possible to have a StatusCode(9000). If not, then creating valid registered status codes with a method that returns a Result could be frustrating.

[carllerche]

Closed by #6