Switch from http-lwt-client and lwt to httpcats and miou #4
Conversation
| | `Malformed_json of string | ||
| | `Msg of string | ||
| | Httpcats.error |
There was a problem hiding this comment.
Do users of httpcats really need a list of errors that detailed?
There was a problem hiding this comment.
I think it's pretty good and it fits in quite well with your code 👍. A transformation could be reword_error (msgf '%a' Httpcats.pp_error) to produce only a value of type [> Msg of string ]. But specifying whether the error comes from h1 or h2 or the underlying protocol (tcp or tls) or the occurrence of an unknown exception (often raised by the user (him/her)self) can be useful in more precise cases.
| | Ok (resp, body) -> Error (`Api_error (resp, body)) | ||
| | Error e -> Error e | ||
| | Error e -> Error (e : Httpcats.error :> [> fetch_errors]) |
There was a problem hiding this comment.
I feel like this should be improved in httpcats. Httpcats.request should return (_, [> error]) result instead of the non polymorphic version of that currently.
| @@ -350,15 +358,19 @@ module Config = struct | |||
| Json.pp fmt json | |||
| end | |||
|
|
|||
| external reraise : exn -> 'a = "%reraise" | |||
There was a problem hiding this comment.
This might be nice to have in Miou (or in the stdlib but since the stdlib doesn't have it currently it would be nice to have anyway for backward compatibility)
| let fd = Unix.openfile (Fpath.to_string output_file) Unix.[O_WRONLY; O_TRUNC; O_CREAT] 0o666 in | ||
| let fd = try Miou_unix.of_file_descr fd with e -> Unix.close fd; reraise e in | ||
| Fun.protect ~finally:(fun () -> Miou_unix.close fd) begin fun () -> | ||
| Miou_unix.write fd x; | ||
| end; |
There was a problem hiding this comment.
I feel like there is a simpler way to do that. Could Miou_unix provide an openfile function or would it be more something for a higher level library? If it's the latter, is that already planned? If not i'm happy to start one
| ~uri:url | ||
| (* TODO: This won't work once we handle things that aren't just short and simple JSON *) | ||
| ~f:(fun _ _ acc body -> match acc, body with |
There was a problem hiding this comment.
I feel like the new labels (uri and f) are unnecessary too
There was a problem hiding this comment.
The f label is, indeed, not as important as all that, but the uri label is important because it indicates that the string passed is actually interpreted as an uri. The string type can easily be a catch-all type (just look at the OCaml codebase...) and being able to specify that the string expected by request is the uri is I think a good approach.
Experimental
cc @dinosaure