Skip to content

raise (Failure ..) -> failwith .. #245

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.

Sign up for GitHub

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

Merged
rgrinberg merged 1 commit into from
Jan 27, 2015
Merged

raise (Failure ..) -> failwith .. #245

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions async/cohttp_async.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ module Net = struct
let lookup uri =
let host = Option.value (Uri.host uri) ~default:"localhost" in
match Uri_services.tcp_port_of_uri ~default:"http" uri with
| None -> raise (Failure "Net.connect") (* TODO proper exception *)
| None -> failwith "Net.connect" (* TODO proper exception *)
| Some port ->
let open Unix in
Addr_info.get ~host [Addr_info.AI_FAMILY PF_INET; Addr_info.AI_SOCKTYPE SOCK_STREAM]
>>= function
| {Addr_info.ai_addr=ADDR_INET(addr,_)}::_ ->
return (host, Ipaddr_unix.of_inet_addr addr, port)
| _ -> raise (Failure "resolution failed")
| _ -> failwith "resolution failed"
Copy link
Copy Markdown

@dbuenzli dbuenzli Jan 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather say you are using exceptions for something unexceptional.

Copy link
Copy Markdown
Member Author

@rgrinberg rgrinberg Jan 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes all of this stuff is very much less than ideal. I think we are saving the refactoring post 1.0 however.

Copy link
Copy Markdown
Member

@hannesm hannesm Jan 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why post 1.0? shouldn't such API changes (I suspect raise will be transformed to use an 'a option return type instead) be before 1.0?

Copy link
Copy Markdown
Member Author

@rgrinberg rgrinberg Jan 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we did ask people to participate in making a checklist for 1.0:

#198

and the issue wasn't included. I guess it's not too late now :P.

EDIT: Net isn't being exported so we can easily fix it now actually.

Copy link
Copy Markdown
Member Author

@rgrinberg rgrinberg Jan 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hannesm @dbuenzli #247 addresses this


let connect_uri ?interrupt uri =
lookup uri >>= fun (host, addr, port) ->
Expand Down Expand Up @@ -176,8 +176,8 @@ module Client = struct
>>= fun () ->
Response.read ic
>>| function
| `Eof -> raise (Failure "Connection closed by remote host")
| `Invalid reason -> raise (Failure reason)
| `Eof -> failwith "Connection closed by remote host"
| `Invalid reason -> failwith reason
| `Ok res ->
(* Build a response pipe for the body *)
let reader = Response.make_body_reader res ic in
Expand Down