ocaml-cohttp icon indicating copy to clipboard operation
ocaml-cohttp copied to clipboard

API inconsistencies between lwt and async

Open superbobry opened this issue 8 years ago • 4 comments

Here's a single example from Server:

val respond_string :
  ?headers:Cohttp.Header.t ->
  status:Cohttp.Code.status_code ->
  body:string -> unit -> (Response.t * Cohttp_lwt_body.t) Lwt.t

and

val respond_with_string :
  ?flush:bool ->
  ?headers:Cohttp.Header.t -> ?code:Cohttp.Code.status_code ->
  string -> response Deferred.t

The two functions are (almost) functionally identical, but they have slightly different names and slightly different parameters. I wonder if this was intentional? i.e. to make the user read the code carefully after the switch to another library.

superbobry avatar Jan 29 '17 12:01 superbobry

Some differences are indeed intentional, but others are just inconsistencies that you've correctly pointed out.

rgrinberg avatar Jan 29 '17 19:01 rgrinberg

Would you accept a PR renaming/deprecating some of these?

superbobry avatar Jan 30 '17 00:01 superbobry

Sure. Just please don't make breaking changes.

A few points:

  • Use the [@@deprecated ..] to deprecate functions
  • Prefer names without "filler" words such with
  • Reordering labelled/optional arguments doesn't count as breakage. So feel free to just make those consistent on the spot.

rgrinberg avatar Jan 30 '17 00:01 rgrinberg

@rgrinberg @superbobry I just created a PR for this issue. I was careful not to introduce any breaking change but did add an optional argument flush to the Lwt.Server.respond_string function. If the changes are approved, I could work on renaming the other functions and their optional/labeled arguments.

gjaldon avatar Feb 14 '17 17:02 gjaldon