Address review comments

This also simplifies the tests a lot by moving the determination on how
to set `suf` and what to expect to the anonymous function.

CHANGES.md

@@ -25,13 +25,12 @@
   letting Yojson create an internal (#134, @Leonidas-from-XIV)
 - Add an optional `suf` keyword argument was added to functions that write
   serialized JSON, thus allowing NDJSON output. Most functions default to not
-  adding any suffix except for `to_file` (#124) and functions writing sequences
-  of values where the default is `\n` (#135, @Lenoidas-from-XIV)
+  adding any suffix except for `to_file` (#124, @panglesd) and functions
+  writing sequences of values where the default is `\n` (#135,
+  @Lenoidas-from-XIV)
 
 ### Change
 
-- The function `to_file` now adds a newline at the end of the generated file. An
-  optional argument allows to return to the original behaviour (#124, @panglesd)
 - The `stream_from_*` and `stream_to_*` functions now use a `Seq.t` instead of a
   `Stream.t`, and they are renamed into `seq_from_*` and `seq_to_*` (@gasche, #131).
 

lib/write.mli

@@ -48,7 +48,8 @@ val to_file :
   string -> t -> unit
   (** Write a compact JSON value to a file.
       See [to_string] for the role of the optional arguments.
-      @param suf is the final character of the output. Newline by default. *)
+      @param suf is a suffix appended to the output Newline by default
+      for POSIX compliance. *)
 
 val to_buffer :
   ?suf:string ->
@@ -63,7 +64,7 @@ val seq_to_string :
   ?suf:string ->
   ?std:bool ->
   t Seq.t -> string
-  (** Write a [suf]-separated sequence of compact one-line JSON values to
+  (** Write a sequence of [suf]-suffixed compact one-line JSON values to
       a string.
       @param suf is the suffix ouf each value written. Newline by default.
       See [to_string] for the role of the optional arguments. *)
@@ -74,7 +75,7 @@ val seq_to_channel :
   ?suf:string ->
   ?std:bool ->
   out_channel -> t Seq.t -> unit
-  (** Write a [suf]-separated sequence of compact one-line JSON values to
+  (** Write a sequence of [suf]-suffixed compact one-line JSON values to
       a channel.
       @param suf is the suffix of each value written. Newline by default.
       See [to_channel] for the role of the optional arguments. *)
@@ -84,7 +85,7 @@ val seq_to_file :
   ?suf:string ->
   ?std:bool ->
   string -> t Seq.t -> unit
-  (** Write a [suf]-separated sequence of compact one-line JSON values to
+  (** Write a sequence of [suf]-suffixed compact one-line JSON values to
       a file.
       @param suf is the suffix of each value written. Newline by default.
       See [to_string] for the role of the optional arguments. *)
@@ -94,7 +95,7 @@ val seq_to_buffer :
   ?std:bool ->
   Buffer.t ->
   t Seq.t -> unit
-  (** Write a [suf]-separated sequence of compact one-line JSON values to
+  (** Write a sequence of [suf]-suffixed compact one-line JSON values to
       an existing buffer.
       @param suf is the suffix of each value written. Newline by default.
       See [to_string] for the role of the optional arguments. *)

test/test_write.ml

@@ -1,60 +1,61 @@
-let to_string () =
-  Alcotest.(check string) __LOC__ Fixtures.json_string (Yojson.Safe.to_string Fixtures.json_value)
+let to_string_tests =
+  let test ?suf expected = 
+    Alcotest.(check string) __LOC__ expected (Yojson.Safe.to_string ?suf Fixtures.json_value)
+  in
+  [
+    "to_string with default settings", `Quick, (fun () -> test Fixtures.json_string);
+    "to_string with newline", `Quick, (fun () -> test ~suf:"\n" Fixtures.json_string_newline);
+    "to_string without newline", `Quick, (fun () -> test ~suf:"" Fixtures.json_string);
+  ]
 
 let to_file_tests =
-  let test ?newline () =
+  let test ?suf expected =
     let output_file = Filename.temp_file "test_yojson_to_file" ".json" in
-    let correction = match newline with
-        | None ->
-          Yojson.Safe.to_file output_file Fixtures.json_value;
-          Fixtures.json_string_newline
-        | Some newline -> 
-          let suf = match newline with true -> "\n" | false -> "" in
-          Yojson.Safe.to_file ~suf output_file Fixtures.json_value;
-          if newline then
-            Fixtures.json_string_newline
-          else
-            Fixtures.json_string
-    in
+    Yojson.Safe.to_file ?suf output_file Fixtures.json_value;
     let file_content =
       let ic = open_in output_file in
       let length = in_channel_length ic in
       let s = really_input_string ic length in
       close_in ic;
       s
     in
-    Alcotest.(check string) __LOC__ correction file_content;
-    Sys.remove output_file
+    Sys.remove output_file;
+    Alcotest.(check string) __LOC__ expected file_content
   in
   [
-    "to_file with default settings", `Quick, (fun () -> test ());
-    "to_file with newline", `Quick, (fun () -> test ~newline:true ());
-    "to_file without newline", `Quick, (fun () -> test ~newline:false ());
+    "to_file with default settings", `Quick, (fun () -> test Fixtures.json_string_newline);
+    "to_file with newline", `Quick, (fun () -> test ~suf:"\n" Fixtures.json_string_newline);
+    "to_file without newline", `Quick, (fun () -> test ~suf:"" Fixtures.json_string);
   ]
 
 (* List.to_seq is not available on old OCaml versions. *)
 let rec list_to_seq = function
   | [] -> (fun () -> Seq.Nil)
   | x :: xs -> (fun () -> Seq.Cons (x, list_to_seq xs))
 
-let seq_to_file () =
-  let output_file = Filename.temp_file "test_yojson_seq_to_file" ".json" in
-  let data = [`String "foo"; `String "bar"] in
-  Yojson.Safe.seq_to_file output_file (list_to_seq data);
-  let read_data =
-    let seq = Yojson.Safe.seq_from_file output_file in
-    let acc = ref [] in
-    Seq.iter (fun v -> acc := v :: !acc) seq;
-    List.rev !acc
+let seq_to_file_tests =
+  let test ?suf () =
+    let output_file = Filename.temp_file "test_yojson_seq_to_file" ".json" in
+    let data = [`String "foo"; `String "bar"] in
+    Yojson.Safe.seq_to_file ?suf output_file (list_to_seq data);
+    let read_data =
+      let seq = Yojson.Safe.seq_from_file output_file in
+      let acc = ref [] in
+      Seq.iter (fun v -> acc := v :: !acc) seq;
+      List.rev !acc
+    in
+    Sys.remove output_file;
+    Alcotest.(check (list Testable.yojson)) "seq_{to,from}_file roundtrip" data read_data
   in
-  Sys.remove output_file;
-  if data <> read_data then
-    (* TODO: it would be nice to use Alcotest.check,
-       but we don't have a 'testable' instance for JSON values. *)
-    Alcotest.fail "seq_{to,from}_file roundtrip failure"
+  [
+    "seq_to_file with default settings", `Quick, (fun () -> test ());
+    "seq_to_file with newline", `Quick, (fun () -> test ~suf:"\n" ());
+    "seq_to_file without newline", `Quick, (fun () -> test ~suf:"" ());
+  ]
 
 let single_json =
-  to_file_tests @ [
-  "to_string", `Quick, to_string;
-  "seq_to_file", `Quick, seq_to_file;
-]
+  List.flatten [
+    to_file_tests;
+    to_string_tests;
+    seq_to_file_tests;
+  ]