Open Fishrock123 opened 2 years ago
Hmph. The CI duly notes that this makes interop with hyper http more difficult (obviously).
@yoshuawuyts Hey I would like to merge this tomorrow if there is no objection
Made one additional change to this... all the enums are #[non_exhaustive]
now so we can add more cases in minors where it fits well
Posted a few bits of feedback, but generally this looks great. Thanks for the substantial amount of work here.
@yoshuawuyts if you are going to write a bunch of typed headers could you please take a look at this first?
@Fishrock123 thanks so much for the ping! - we were actually running into very similar issues around error handling while looking to integrate http-types
into the Azure SDK (https://github.com/Azure/azure-sdk-for-rust/issues/848), and I'm fairly comfortable saying that this seems like the right direction to go.
I still need to look at this line-by-line, but want to at least respond that I agree that we should land this before any further work on typed headers.
I haven't read the diff, but just a quick though since future API breakage seems to be a strong concern here. Consider making all the concrete enums nonexhaustive. That will permit expanding them in future without API breakage - everyone will need to explicitly handle unexpected errors and not assume the error space will expand. (See e.g. filesystem errors in the stdlib for the pain of not doing this in advance).
I haven't read the diff, but just a quick though since future API breakage seems to be a strong concern here. Consider making all the concrete enums nonexhaustive. That will permit expanding them in future without API breakage - everyone will need to explicitly handle unexpected errors and not assume the error space will expand. (See e.g. filesystem errors in the stdlib for the pain of not doing this in advance).
Yes, it will be a large api change. I think that’s fine since no one really likes the current api anyway.
Also, yes I already did mark them all as #[non_exhaustive]
:
https://github.com/http-rs/http-types/pull/395/files#diff-87f966704ebddb378e447e13ddf9ee203cd9d31bb502338a5f3ed3478df41588R9
I'm maintaining a hard fork of this repo that I plan to keep up to date, please resubmit this PR at https://github.com/OneOfOne/http-types-rs
Overhauls errors to allow for 3 types of errors:
Error
was previously)There is some exposition for this in Zulip: https://http-rs.zulipchat.com/#narrow/stream/261414-http-types/topic/New.20error.20types.20.26.20result.20apis/near/254273467
Basically this solves two problems:
The first problem is pretty simple to solve but was quite laborious. It meant changing all those
bail!("some string error")
and what not to be concrete enum variants to some degree of specificity. I have kept some errors grouped together where the string output could mean something to anyone debugging a problem but is unlikely to be something which someone would reasonably want to check during program runtime. This is to reduce the amount of labor doing this but also to cap the enum variants and prevent the number of variants from becoming unmanageable.The second problem is tricky and weird. My solution is
RequestError
. This allows dynamic errors from surf middleware but also allows us to have concrete errors there. Some indirection is required forResponseError
due toStdError
's conflict withanyhow
(The problem is a conflict withFrom<T> for T
, which I will never not be profusely annoyed at).There are some potential issues here / open questions:
HTTP(http_types::Error)
or such, which... isn't really correct.HeaderError
,BodyError
, etc. Would need to make a couple extra types.Error::ArgTryIntoError(Box<dyn std::error::Error + Send + Sync + 'static>)
exist? Would returningResult<Result<T>>
in the necessary places be better? (@yoshuawuyts this is what I was asking on Zulip)Send + Sync + 'static
requirements correct or a problem?Edit: the diff is obviously quite large, so here are the interesting parts:
errors::error_kind.rs
errors::mod.rs
errors::request_error.rs
errors::response_error.rs