Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a configurable suf separator #135

Merged
merged 1 commit into from
Mar 3, 2022
Merged

Add a configurable suf separator #135

merged 1 commit into from
Mar 3, 2022

Conversation

Leonidas-from-XIV
Copy link
Member

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.

@Leonidas-from-XIV Leonidas-from-XIV added this to the 2.0.0 milestone Feb 28, 2022
@Leonidas-from-XIV Leonidas-from-XIV changed the title Add a confugurable sep separator Add a configurable sep separator Feb 28, 2022
@panglesd
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Member Author

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.

@Leonidas-from-XIV Leonidas-from-XIV marked this pull request as ready for review February 28, 2022 15:52
@panglesd
Copy link
Collaborator

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 Leonidas-from-XIV changed the title Add a configurable sep separator Add a configurable suf separator Mar 2, 2022
@Leonidas-from-XIV
Copy link
Member Author

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
panglesd reviewed Mar 3, 2022
View reviewed changes
Copy link
Collaborator

@panglesd panglesd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
lib/write.mli Outdated Show resolved Hide resolved
lib/write.mli Outdated Show resolved Hide resolved
lib/write.mli Outdated Show resolved Hide resolved
lib/write.mli Outdated Show resolved Hide resolved
lib/write.mli Outdated Show resolved Hide resolved
test/test_write.ml Outdated Show resolved Hide resolved
panglesd
panglesd approved these changes Mar 3, 2022
View reviewed changes
Copy link
Collaborator

@panglesd panglesd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CHANGES.md Outdated Show resolved Hide resolved
@Leonidas-from-XIV
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 Leonidas-from-XIV merged commit e2a85dc into ocaml-community:master Mar 3, 2022
@Leonidas-from-XIV Leonidas-from-XIV deleted the separator branch March 3, 2022 15:37
panglesd added a commit to panglesd/opam-repository that referenced this pull request Jun 2, 2022
@panglesd
[new release] yojson and yojson-bench (2.0.0)
dea293d 
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)
@panglesd panglesd mentioned this pull request Jun 2, 2022
Merged
panglesd added a commit to panglesd/opam-repository that referenced this pull request Jun 3, 2022
@panglesd
[new release] yojson and yojson-bench (2.0.0)
b1b0fac 
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 pushed a commit to panglesd/opam-repository that referenced this pull request Jun 9, 2022
@panglesd @Leonidas-from-XIV
[new release] yojson and yojson-bench (2.0.0)
accb6b8 
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panglesd panglesd panglesd approved these changes

@NathanReb NathanReb Awaiting requested review from NathanReb NathanReb is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Leonidas-from-XIV @panglesd