Added Header.add_multi

[SGrondin]

I think it's cleaner than lines of

let headers = Header.init ()
  |> fun h -> Header.add h "something" "something"
  |> fun h -> Header.add h "something" "something"
  |> fun h -> Header.add h "something" "something"
(* etc *)

when adding multiple headers at once.

Discuss?

[avsm]

Makes sense to me. It would be nice to have put the h argument to the end of Header to facilitate better currying, but too late for this iteration of the API.

[rgrinberg]

+1 from me

[dbuenzli]

The name is very ugly, add_list ?

[avsm]

multiple is the term used in RFC2616;

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)].

So add_multi expresses this more clearly than add_list (i.e. that duplicates are permissible). It could be better to explicitly call this add_multiple rather than contract it...

[dbuenzli]

Personally what I read is what they refer as multiple message header fields are fields that appear more than once:

It MUST be possible to combine the multiple header fields into one
   "field-name: field-value" pair, without changing the semantics of the
   message, by appending each subsequent field-value to the first, each
   separated by a comma.

by that count the signature of a potential add_multiple would rather be:

add_multiple : t -> string -> string list -> t

no ? Also, shouldn't that module be called Headers ?

[dbuenzli]

by that count the signature of a potential add_multiple would rather be:

And thus to be pedantic the function of the PR should be called add_multiples (plural), at which point I think I would prefer add_list and document the fact that duplicates are acceptable.

[SGrondin]

It's named add_multi because of

ocaml-cohttp/lib/header.mli

Line 66 in bccaff2

val get_multi : t -> string -> string list

. I understand @dbuenzli 's point about _multi being for header keys that have multiple values, but this PR lets users add multiple values to a key, too. In order to keep _multi for functions that ONLY deal with multiple values per key, I think add_list is the best alternative.

Also, I stayed with the convention of having h as the first argument. I would have liked to be able to do Header.init () |> Header.add_multi mylist, but maybe it'll be in version 2 when the API is improved..

[dbuenzli]

@SGrondin good point didn't know about get_multi (that's what happens when you comment on things without having a deeper look, sorry) I think you should then have:

val add_multi : t -> string -> string list -> t
val add_list : t -> (string * string) list -> t

add_list is also entirely consistant with of_list and to_list.

Also it seem to me that the signature of get_multi is wrong it should return an optional list. With the current signature you cannot distinguish between absence of a multiheader and an empty multiheader.

[dsheets]

Multiheaders have a list of values separated by commas for a key or multiple equal keys or both. If get_multi h k is [] then there does not exist a header k in the set h. If you had an empty header, it would result in [""] or ["";""] for multiple empty headers (or, I suppose, a comma as a value).

[dbuenzli]

@dsheets right, you may want to count the number of empty occurences of the same header. (Though the comma thing should actually be sorted out, if you don't keep commas when there are values you should not put one when there is no value).

[dbuenzli]

(This shows that these functions should actually be documented w.r.t. to these details, I can save you a lot of time).

[SGrondin]

Alright, so is there any consensus between add_multi and add_list?

[dsheets]

The number of empty occurrences will be the number of empty strings. A multiheader with value of comma should result in two empty occurrences.

I'm not sure you understand my point about commas: if there is no value, no comma is generated; if the header is not a multiheader, a comma doesn't mean anything and is part of the value; if there is a value for a multiheader and it is only a comma, this is equivalent to two empty multiheaders.

I think this behavior is fairly intuitive. There could probably be more documentation but these details should be obvious. I'm not sure where the confusion is.

@SGrondin I agree with @dbuenzli's names and signatures fwiw.

[SGrondin]

I implemented

val add_multi : t -> string -> string list -> t
val add_list : t -> (string * string) list -> t

please review it.

[rgrinberg]

Makes sense to me. It would be nice to have put the h argument to the end of Header to facilitate better currying, but too late for this iteration of the API.

The other option being to add labels to the other params. Aesthetically, this is what I prefer but I realize it could be just better to follow the existing convention (which is t last).

In any case, I think there's consensus on the names of the 2 functions and since the implementation is trivial enough I'm merging this in.