Support accessing the response code as an integer

[simpkins]

The Status enum only supports a portion of the response codes defined by RFC 9110. If the server returned a response code other than one of the ones supported by the Status enum, the code previously threw away the value. This updates the code to also store the code as an integer, so that users can handle other values.

I considered simply changing the Unknown variant to use Unknown(u16), but avoided it for now since this would break backwards compatibility, and would also prevent using status as u16 to convert Status values to integers.

[rmja]

I would probably prefer an Other variant on the Status enum to avoid having two apis to convey the same information.

[simpkins]

I would probably prefer an Other variant on the Status enum to avoid having two apis to convey the same information.

Sure, I can do that instead if you prefer. I agree that it does make the API a bit nicer. That said, it does have some downsides since it breaks backwards compatibility, and makes the Status enum take more space to store and no longer convertible to u16 with the as u16 syntax. It also results in slightly weird comparison behavior (e.g., should Status::Ok and Status::Other(200) be treated as equal?).

[rmja]

Yes, that can lead to subtle bugs, as every addition of a known status variant is effectively a breaking change. This is actually the reason why I have not already made the change:) @lulf what do you think?

[lulf]

It's unfortunate it requires breaking, but I also don't think there's been any promises made to not break the API, so might as well do it now. Maybe there should be a split where the StatusCode is just u16 wrapper, and used in the structs, whereas the Status enum is a convenience that can be Into in any input APIs.

[rmja]

The thing is that we will have a breaking change every time that a new known status code is added. Consider this code:

if let Other(status) = response.status && status == 123. That code could suddenly fail if 123 is later added as a known status code.

Another option would be to simply expose a status: u16 on the response replacing the current status: Status enum, and then have a get_status(): Status function that could try and convert the status: u16 to the known variants in Status. That would not cause any breaking changes later if we let the Unknown variant stay instead of introducing an Other(u16).

[lulf]

The thing is that we will have a breaking change every time that a new known status code is added. Consider this code:

if let Other(status) = response.status && status == 123. That code could suddenly fail if 123 is later added as a known status code.

Another option would be to simply expose a status: u16 on the response replacing the current status: Status enum, and then have a get_status(): Status function that could try and convert the status: u16 to the known variants in Status. That would not cause any breaking changes later if we let the Unknown variant stay instead of introducing an Other(u16).

The thing is that we will have a breaking change every time that a new known status code is added. Consider this code:

if let Other(status) = response.status && status == 123. That code could suddenly fail if 123 is later added as a known status code.

Another option would be to simply expose a status: u16 on the response replacing the current status: Status enum, and then have a get_status(): Status function that could try and convert the status: u16 to the known variants in Status. That would not cause any breaking changes later if we let the Unknown variant stay instead of introducing an Other(u16).

Sorry for being unclear, I think we agree. This is what I meant in my previous comment as well, through a newtype StatusCode (u16) instead of u16. But using u16 directly is perhaps better. Having an get_status() to attempt the conversion makes sense 👍

[rmja]

Ahh yes! StatusCode(u16) is a good idea, I like it! We can have that together with a get_status(): Status on the response where we keep the Status:Unknown variant. That should not break anything going forward. We could also implement Into<Status> for StatusCode and TryFrom<StatusCode> for Status for. @simpkins does this make sense? Sorry for the confusion here. Feel free to make this, the pr is already approved:)

[simpkins]

Sure, I can make those changes.

We could also implement Into<Status> for StatusCode and TryFrom<StatusCode> for Status

Did you perhaps mean TryFrom<Status> for StatusCode and From<StatusCode> for Status?

I did initially take a stab at providing TryFrom<StatusCode> for Status rather than From<StatusCode>, but this makes the upgrade process more awkward for downstream code, since they can't just say response.status.into() to upgrade existing sites expecting a Status enum.

[rmja]

Looks perfect, thank you! I think that you can merge yourself when you are ready.

[simpkins]

Looks perfect, thank you! I think that you can merge yourself when you are ready.

Thanks. It looks like the CI actions are stuck somehow though, so I don't think I can merge it yet. It's reporting the CI / ci (pull_request) action as still queued. The most recent run succeeded, but I wonder if it is somehow still waiting on this old one?

[lulf]

I re-ran the action, seemed to do the trick!