Skip to content
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

Merged
merged 2 commits into from
Apr 19, 2023
Merged

Introduce Absolute_http specialization #164

merged 2 commits into from
Apr 19, 2023

Conversation

torinnd
Copy link
Contributor

@torinnd torinnd commented Nov 25, 2022

An RFC9110-compliance specialization of a [Uri.t]

Addresses #162

@avsm
Copy link
Member

avsm commented Jan 17, 2023

Thanks! This looks good to me, some notes in no order:

  • needs some tests as well
  • we're increasingly shifting to exception raising functions as the default, with the move towards direct-style interfaces in OCaml 5. In this case, the exception raised should probably be more specific (not Failure), and I wonder if the Result type should be the slow one (that is, implement the function to raise an effect, and wrap that for the Result version of the function rather than the other way around).

@talex5
Copy link

talex5 commented Jan 17, 2023

we're increasingly shifting to exception raising functions as the default, with the move towards direct-style interfaces in OCaml 5.

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. Eio.Buf_read.parse returns a result type, because it's common to want to handle parsing errors. But it wraps individual parsers that raise Failure on error, because plumbing result types through all the parsing code is slow.

This isn't my library, but this PR's choice of returning a result for the new of_uri function, and matching what the rest of the library does for of_string, seems fine to me. It would be good to document what of_string does on error, though.

@dinosaure
Copy link
Member

dinosaure commented Jan 17, 2023

This isn't my library, but this PR's choice of returning a result for the new of_uri function, and matching what the rest of the library does for of_string, seems fine to me. It would be good to document what of_string does on error, though.

I agree with that and ocaml-uri is relatively standalone. We should not use a wider error politic other than the one described into our website.

@avsm
Copy link
Member

avsm commented Jan 17, 2023

@talex5 wrote:

But it wraps individual parsers that raise Failure on error, because plumbing result types through all the parsing code is slow

Right, that's what made me think that the exception should be the fast one, as callers can then aggregate them into a single result.

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

@torinnd
Copy link
Contributor Author

torinnd commented Jan 24, 2023

I've taken a stab at putting a test harness together, but am bumping into the following ocaml-ci error:
2023-01-24 15:01.16: Job failed: Error from solver: Unix.Unix_error(Unix.EMFILE, "pipe", "")

I'm not sure if this is an issue with the PR or the CI tooling

@talex5
Copy link

talex5 commented Jan 24, 2023

2023-01-24 15:01.16: Job failed: Error from solver: Unix.Unix_error(Unix.EMFILE, "pipe", "")
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.

@talex5
Copy link

talex5 commented Jan 24, 2023

The new error is likely a problem with the PR however:

File "lib_test/test_runner.ml", line 727, characters 33-37:
727 |   eval_rfc9110_uris http_uris ~f:eval
                                       ^^^^
Error: This expression has type
         input:string -> (Uri.Absolute_http.t, string) result -> unit
       but an expression was expected of type
         input:string ->
         (Uri.Absolute_http.t, [ `Msg of string ]) result -> unit
       Type string is not compatible with type [ `Msg of string ] 

@tmcgilchrist
Copy link
Member

@torinnd the CI issue is resolved, could you try rebasing this PR. Thanks

@torinnd
Copy link
Contributor Author

torinnd commented Mar 21, 2023

Checks cleared! Thanks for getting the opam issue sorted @tmcgilchrist !

@torinnd
Copy link
Contributor Author

torinnd commented Apr 10, 2023

@avsm are you open to merging this? I think we've sorted all of the feedback raised in this PR.

@avsm avsm merged commit b941fc9 into mirage:master Apr 19, 2023
avsm added a commit to avsm/opam-repository that referenced this pull request Apr 19, 2023
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).
@avsm
Copy link
Member

avsm commented Apr 19, 2023

Merged, and released to opam. Thanks for the contribution!

kit-ty-kate pushed a commit to avsm/opam-repository that referenced this pull request Apr 26, 2023
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants