-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce Absolute_http specialization #164
Conversation
Thanks! This looks good to me, some notes in no order:
|
If you mean Eio's error handling policy then I'd like to clarify that this is for IO errors, where there is a large and unknown set of possible errors (e.g. depending on the platform). For those, you can't handle all individual errors and must usually propagate them by default, so exceptions typically make more sense. Parsing is an interesting case. This isn't my library, but this PR's choice of returning a result for the new |
I agree with that and |
@talex5 wrote:
Right, that's what made me think that the exception should be the fast one, as callers can then aggregate them into a single But this can all be fixed later with affecting the interfaces in this PR. I'm good with merging this if you could add some tests, @torinnd |
I've taken a stab at putting a test harness together, but am bumping into the following ocaml-ci error: I'm not sure if this is an issue with the PR or the CI tooling |
It's a problem with the CI certainly. The CI team are aware of it. |
The new error is likely a problem with the PR however:
|
@torinnd the CI issue is resolved, could you try rebasing this PR. Thanks |
Checks cleared! Thanks for getting the opam issue sorted @tmcgilchrist ! |
@avsm are you open to merging this? I think we've sorted all of the feedback raised in this PR. |
CHANGES: * Add `Uri.Absolute_http`, an RFC9110-compliance specialization of a `Uri.t`. (mirage/ocaml-uri#164 mirage/ocaml-uri#162 @torinnd). * Add a `uri-bench` package for the benchmarking dependencies in this repository (mirage/ocaml-uri#166 @tmcgilchrist).
Merged, and released to opam. Thanks for the contribution! |
CHANGES: * Add `Uri.Absolute_http`, an RFC9110-compliance specialization of a `Uri.t`. (mirage/ocaml-uri#164 mirage/ocaml-uri#162 @torinnd). * Add a `uri-bench` package for the benchmarking dependencies in this repository (mirage/ocaml-uri#166 @tmcgilchrist).
An RFC9110-compliance specialization of a [Uri.t]
Addresses #162