API inconsistencies between lwt and async

mirage / ocaml-cohttp

An OCaml library for HTTP clients and servers using Lwt or Async

Other

711 stars 174 forks source link

API inconsistencies between lwt and async #524

Open superbobry opened 7 years ago

superbobry commented 7 years ago

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.

rgrinberg commented 7 years ago

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

superbobry commented 7 years ago

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

rgrinberg commented 7 years ago

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.

gjaldon commented 7 years ago

@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.