Add a configurable `suf` separator by Leonidas-from-XIV · Pull Request #135 · ocaml-community/yojson

Leonidas-from-XIV · 2022-02-28T14:23:40Z

At the moment the documentation claims that what we output is newline separated (which it isn't). As such we should probably either fix the documentation or the implementation.

In this PR I chose to go for the second thing. @panglesd added a way to configure whether a newline is added or not in #124 but since 2.0.0 is not yet released I thought it would be nice to generalize it to all the functions, especially if people don't want the separators.

I chose sep as the name of the argument, because it is a common name. Notably though it is different from sep as used in Fmt in that it is always generated in the end. The reason for this is that the semi-official ndjson spec specifies that the JSON always ends with a newline so having ~sep:"\n" achieves exactly that behavior.

panglesd · 2022-02-28T15:12:28Z

I think that sep should be used for a string that will be put in between two entries, but not at the end.
For instance, StringLabels.concat ~sep:"a" ["b";"c"] will output ""bac" and not "baca".

I think that your PR adds sep as a trailing string to each entry. (Am I wrong? I did not test, I just had a look at the code)

I am not sure if we prefer the "true" sep behaviour or to change the name of the parameter to trailing or something like that, but the name should reflect the behaviour as much as possible!

Leonidas-from-XIV · 2022-02-28T15:22:31Z

You're right, it is just an ending (otherwise it would also generate invalid NDJSON) and fortunately also easier to implement. I chose sep as a name because that's a pretty common name but I am not insisting. I would just enjoy having something not overly long.

Leonidas-from-XIV · 2022-02-28T15:27:47Z

Also, as you see I made the default value \n everywhere for consistency. For to_file I would argue it makes sense as well as for the functions that output Seq.ts. Not sure that it makes sense for the others, like to_string and to_buffer, so I am open to feedback on that front as well.

panglesd · 2022-02-28T16:48:33Z

What do you think of ~suf for suffixes? It is as short as sep but also meaningful for single values ; and also common I think. For instance, the doc-string of seq_to_string would look like:

Write a [suf]-suffixed sequence of compact one-line JSON values to a string.

On the default value front, I would not have a default value of "\n" for to_string, or a default value "" for to_file, even for consistency.

So I would go for:

to_file has a default value of "\n", as well as other functions that takes Seq.t as input,
other functions have a default value of ""

Of course, all of this documented in the doc!

Leonidas-from-XIV · 2022-03-03T08:52:27Z

I like the idea of suf so I changed the code to use that name. Also, the suggestion to use "\n" in some places (to_file and *_of_seq) which are documented now.

panglesd

Looks good to me!
The only changes required for merge is removing the changelog entry about the optional newline argument, and the doc-comment of to_file. Feel free to ignore the other comments if you don't agree with the suggestions!

CHANGES.md

lib/write.mli

test/test_write.ml

panglesd

Thanks a lot! Feel free to merge, but you might want to correct the typo on your name before ;)

CHANGES.md

This is a generalization of `newline`, but it works for all writing functions and can be overridden.

@c-cube

CHANGES: ### Removed - Removed dependency on easy-format and removed `pretty_format` from `Yojson`, `Yojson.Basic`, `Yojson.Safe` and `Yojson.Raw`. (@c-cube, ocaml-community/yojson#90) - Removed dependency on `biniou`, simplifying the chain of dependencies. This changes some APIs: * `Bi_outbuf.t` in signatures is replaced with `Buffer.t` * `to_outbuf` becomes `to_buffer` and `stream_to_outbuf` becomes `stream_to_buffer` (@Leonidas-from-XIV, ocaml-community/yojson#74, and @gasche, ocaml-community/yojson#132) - Removed `yojson-biniou` library - Removed deprecated `json` type aliasing type `t` which has been available since 1.6.0 (@Leonidas-from-XIV, ocaml-community/yojson#100). - Removed `json_max` type (@Leonidas-from-XIV, ocaml-community/yojson#103) - Removed constraint that the "root" value being rendered (via either `pretty_print` or `to_string`) must be an object or array. (@cemerick, ocaml-community/yojson#121) - Removed `validate_json` as it only made sense if the type was called `json`. (@Leonidas-from-XIV, ocaml-community/yojson#137) ### Add - Add an opam package `yojson-bench` to deal with benchmarks dependency (@tmcgilchrist, ocaml-community/yojson#117) - Add a benchmark to judge the respective performance of providing a buffer vs letting Yojson create an internal (ocaml-community/yojson#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` (ocaml-community/yojson#124, @panglesd) and functions writing sequences of values where the default is `\n` (ocaml-community/yojson#135, @Leonidas-from-XIV) ### Change - 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, ocaml-community/yojson#131). ### Fix - Avoid copying unnecessarily large amounts of strings when parsing (ocaml-community/yojson#85, ocaml-community/yojson#108, @Leonidas-from-XIV) - Fix `stream_to_file` (ocaml-community/yojson#133, @tcoopman and @gasche)

@c-cube

CHANGES: ### Removed - Removed dependency on easy-format and removed `pretty_format` from `Yojson`, `Yojson.Basic`, `Yojson.Safe` and `Yojson.Raw`. (@c-cube, ocaml-community/yojson#90) - Removed dependency on `biniou`, simplifying the chain of dependencies. This changes some APIs: * `Bi_outbuf.t` in signatures is replaced with `Buffer.t` * `to_outbuf` becomes `to_buffer` and `stream_to_outbuf` becomes `stream_to_buffer` (@Leonidas-from-XIV, ocaml-community/yojson#74, and @gasche, ocaml-community/yojson#132) - Removed `yojson-biniou` library - Removed deprecated `json` type aliasing type `t` which has been available since 1.6.0 (@Leonidas-from-XIV, ocaml-community/yojson#100). - Removed `json_max` type (@Leonidas-from-XIV, ocaml-community/yojson#103) - Removed constraint that the "root" value being rendered (via either `pretty_print` or `to_string`) must be an object or array. (@cemerick, ocaml-community/yojson#121) - Removed `validate_json` as it only made sense if the type was called `json`. (@Leonidas-from-XIV, ocaml-community/yojson#137) ### Add - Add an opam package `yojson-bench` to deal with benchmarks dependency (@tmcgilchrist, ocaml-community/yojson#117) - Add a benchmark to judge the respective performance of providing a buffer vs letting Yojson create an internal (ocaml-community/yojson#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` (ocaml-community/yojson#124, @panglesd) and functions writing sequences of values where the default is `\n` (ocaml-community/yojson#135, @Leonidas-from-XIV) ### Change - 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, ocaml-community/yojson#131). ### Fix - Avoid copying unnecessarily large amounts of strings when parsing (ocaml-community/yojson#85, ocaml-community/yojson#108, @Leonidas-from-XIV) - Fix `stream_to_file` (ocaml-community/yojson#133, @tcoopman and @gasche)

@c-cube

CHANGES: ### Removed - Removed dependency on easy-format and removed `pretty_format` from `Yojson`, `Yojson.Basic`, `Yojson.Safe` and `Yojson.Raw`. (@c-cube, ocaml-community/yojson#90) - Removed dependency on `biniou`, simplifying the chain of dependencies. This changes some APIs: * `Bi_outbuf.t` in signatures is replaced with `Buffer.t` * `to_outbuf` becomes `to_buffer` and `stream_to_outbuf` becomes `stream_to_buffer` (@Leonidas-from-XIV, ocaml-community/yojson#74, and @gasche, ocaml-community/yojson#132) - Removed `yojson-biniou` library - Removed deprecated `json` type aliasing type `t` which has been available since 1.6.0 (@Leonidas-from-XIV, ocaml-community/yojson#100). - Removed `json_max` type (@Leonidas-from-XIV, ocaml-community/yojson#103) - Removed constraint that the "root" value being rendered (via either `pretty_print` or `to_string`) must be an object or array. (@cemerick, ocaml-community/yojson#121) - Removed `validate_json` as it only made sense if the type was called `json`. (@Leonidas-from-XIV, ocaml-community/yojson#137) ### Add - Add an opam package `yojson-bench` to deal with benchmarks dependency (@tmcgilchrist, ocaml-community/yojson#117) - Add a benchmark to judge the respective performance of providing a buffer vs letting Yojson create an internal (ocaml-community/yojson#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` (ocaml-community/yojson#124, @panglesd) and functions writing sequences of values where the default is `\n` (ocaml-community/yojson#135, @Leonidas-from-XIV) ### Change - 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, ocaml-community/yojson#131). ### Fix - Avoid copying unnecessarily large amounts of strings when parsing (ocaml-community/yojson#85, ocaml-community/yojson#108, @Leonidas-from-XIV) - Fix `stream_to_file` (ocaml-community/yojson#133, @tcoopman and @gasche)

Leonidas-from-XIV added this to the 2.0.0 milestone Feb 28, 2022

Leonidas-from-XIV changed the title ~~Add a confugurable sep separator~~ Add a configurable sep separator Feb 28, 2022

Leonidas-from-XIV marked this pull request as ready for review February 28, 2022 15:52

Leonidas-from-XIV requested review from NathanReb and panglesd as code owners February 28, 2022 15:52

Leonidas-from-XIV changed the title ~~Add a configurable sep separator~~ Add a configurable suf separator Mar 2, 2022

Leonidas-from-XIV force-pushed the separator branch from 36460c8 to 6636035 Compare March 2, 2022 13:28

panglesd reviewed Mar 3, 2022

View reviewed changes

Leonidas-from-XIV requested a review from panglesd March 3, 2022 13:30

panglesd approved these changes Mar 3, 2022

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

Add suf separator to configure what terminates JSON values

0330104

This is a generalization of `newline`, but it works for all writing functions and can be overridden.

Leonidas-from-XIV force-pushed the separator branch from e8e28bc to 0330104 Compare March 3, 2022 15:09

Leonidas-from-XIV merged commit e2a85dc into ocaml-community:master Mar 3, 2022

Leonidas-from-XIV deleted the separator branch March 3, 2022 15:37

panglesd mentioned this pull request Jun 2, 2022

[new release] yojson and yojson-bench (2.0.0) ocaml/opam-repository#21460

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a configurable `suf` separator#135

Add a configurable `suf` separator#135
Leonidas-from-XIV merged 1 commit intoocaml-community:masterfrom
Leonidas-from-XIV:separator

Leonidas-from-XIV commented Feb 28, 2022

Uh oh!

panglesd commented Feb 28, 2022

Uh oh!

Leonidas-from-XIV commented Feb 28, 2022

Uh oh!

Leonidas-from-XIV commented Feb 28, 2022

Uh oh!

panglesd commented Feb 28, 2022

Uh oh!

Leonidas-from-XIV commented Mar 3, 2022

Uh oh!

panglesd left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

panglesd left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Leonidas-from-XIV commented Feb 28, 2022

Uh oh!

panglesd commented Feb 28, 2022

Uh oh!

Leonidas-from-XIV commented Feb 28, 2022

Uh oh!

Leonidas-from-XIV commented Feb 28, 2022

Uh oh!

panglesd commented Feb 28, 2022

Uh oh!

Leonidas-from-XIV commented Mar 3, 2022

Uh oh!

panglesd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

panglesd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants