8000 Remove Tuple & Variant by Leonidas-from-XIV · Pull Request #158 · ocaml-community/yojson · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove Tuple & Variant #158

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
Merged
merged 7 commits into from
Jun 28, 2024
Merged

Remove Tuple & Variant #158

merged 7 commits into from
Jun 28, 2024

Conversation

Leonidas-from-XIV
Copy link
Member

It has been long time on my mind but now that Yojson 2.x is finally out, we can start looking at the next step, Yojson 3.0 (which I assume should be a fairly minor issue issue, given most consumers probably don't ever deal with `Tuple or `Variant).

The code itself is pretty self-explanatory, it mostly eliminates the TUPLE and VARIANT CPPO variables and then deletes some support code.

There might be more code to be deleted, like start_any_variant/start_any_tuple. I assume these might be used by atd (need to look into it) so I just removed the part that parses non-standard syntax but maybe it can be removed altogether, along with the unit test that covers it.

Closes #104

@Leonidas-from-XIV
Copy link
Member Author

A bit of research has shown that atdgen generates both start_any_variant with the `Edgy_bracket case as well as start_any_tuple, so the reading code there would most likely need to be adapted.

@Leonidas-from-XIV Leonidas-from-XIV mentioned this pull request Mar 29, 2023
Open
hhugo
hhugo reviewed Jul 21, 2023
View reviewed changes
hhugo
hhugo reviewed Jul 21, 2023
View reviewed changes
hhugo
hhugo reviewed Jul 21, 2023
View reviewed changes
hhugo
hhugo reviewed Jul 21, 2023
View reviewed changes
hhugo
hhugo reviewed Jul 21, 2023
View reviewed changes
hhugo
hhugo reviewed Jul 21, 2023
View reviewed changes
@Leonidas-from-XIV
Remove unused cases
73a5166 
Thanks to @hhugo for pointing them out
@Leonidas-from-XIV
Remove dead code
8bbbc1f 
This seems to be unused in Yojson and Atd
@Leonidas-from-XIV Leonidas-from-XIV added this to the 3.0.0 milestone May 31, 2024
@c-cube
Copy link
Member
c-cube commented May 31, 2024 •
edited
Loading

Is this going to kill ppx_deriving_yojson ? I think it targets the safe fragment so it might expect Tuple and Variant as inputs…

@Leonidas-from-XIV
Copy link
Member Author

@c-cube I don't think it will break ppx_deriving_yojson too bad, as it encodes tuples as lists and variants as lists with the name of the constructor as first argument, if I remember correctly. Here's the code: https://github.com/ocaml-ppx/ppx_deriving_yojson/blob/61f2d63138ec1efea0a3fb4afa682f40497ec380/src/ppx_deriving_yojson.ml#L115-L148

The biggest (only?) issue will probably be Atd which uses Tuple and Variant, but I believe it would make sense in that case to adopt the same encoding strategy that ppx_deriving_yojson uses. Which has the advantage that the JSON it generates is standard-compliant and thus more interoperable with non-OCaml tools.

@mjambon mjambon mentioned this pull request Jun 3, 2024
Open
@mjambon
Copy link
Member
mjambon commented Jun 3, 2024 •
edited
Loading

The biggest (only?) issue will probably be Atd which uses Tuple and Variant

I don't think it's a big issue, and it's a good move forward.

Atdgen will make its -j-std option the default, and the old default behavior that was supporting the experimental syntax for tuples and variants will be removed.

To see the dependencies on Yojson functions, I used this:

~/atd/atdgen/src $ git grep -oP 'Yojson.[^" ]+' | sort | uniq -c
      2 ocaml.ml:Yojson.Safe.t
      1 oj_emit.ml:Yojson.End_of_object
      5 oj_emit.ml:Yojson.End_of_tuple
      1 oj_emit.ml:Yojson.Safe.finish_string
      1 oj_emit.ml:Yojson.Safe.init_lexer
      2 oj_emit.ml:Yojson.Safe.lexer_state
      1 oj_emit.ml:Yojson.Safe.map_ident
      1 oj_emit.ml:Yojson.Safe.read_comma
      2 oj_emit.ml:Yojson.Safe.read_gt
      1 oj_emit.ml:Yojson.Safe.read_ident
      1 oj_emit.ml:Yojson.Safe.read_lcurl
      2 oj_emit.ml:Yojson.Safe.read_null_if_possible
      1 oj_emit.ml:Yojson.Safe.read_object_end
      1 oj_emit.ml:Yojson.Safe.read_object_sep
      1 oj_emit.ml:Yojson.Safe.read_rbr
     16 oj_emit.ml:Yojson.Safe.read_space
      1 oj_emit.ml:Yojson.Safe.read_tuple_end2
      4 oj_emit.ml:Yojson.Safe.read_tuple_sep2
      2 oj_emit.ml:Yojson.Safe.skip_json
      1 oj_emit.ml:Yojson.Safe.start_any_tuple
      1 oj_emit.ml:Yojson.Safe.start_any_variant
      1 oj_emit.ml:Yojson.Safe.to_string
      1 oj_emit.ml:Yojson.Safe.write_bool
      1 oj_emit.ml:Yojson.Safe.write_float
      1 oj_emit.ml:Yojson.Safe.write_float_prec
      1 oj_emit.ml:Yojson.Safe.write_int
      1 oj_emit.ml:Yojson.Safe.write_json
      1 oj_emit.ml:Yojson.Safe.write_null
      1 oj_emit.ml:Yojson.Safe.write_std_float
      1 oj_emit.ml:Yojson.Safe.write_std_float_prec
      1 oj_emit.ml:Yojson.Safe.write_string
      1 omelange_emit.ml:Yojson.Safe.to_string

Occurrences such as Yojson.End_of_tuple, Yojson.Safe.read_tuple_end2, and a few others will have to be removed.

See ahrefs/atd#412

@Leonidas-from-XIV
Remove variant code
1b7083f
@Leonidas-from-XIV
Remove map_string from API
d441f4f 
Its not tested anymore and unused by Yojson
@Leonidas-from-XIV
Copy link
Member Author

@mjambon That's a great list, will be very handy!

I've been looking at the std variants and with the removal of tuple and variant the only non-standard thing seems to be the printing of floats (NaN, +Infinity, -Infinity). I see that atdgen is currently emitting both write_float(_prec) and write_std_float(_prec). Looking at atdgen it seems to be configurable (p.std) which variant it will use.

I'm thinking that as we're breaking compatibility anyway to become more JSON-compliant, might be sensible to bite the bullet and make the std variants the only option, thus failing to encode unrepresentable floats in JSON. WDYT?

@Leonidas-from-XIV Leonidas-from-XIV merged commit ce6e250 into ocaml-community:master Jun 28, 2024
3 of 4 checks passed
@Leonidas-from-XIV Leonidas-from-XIV deleted the remove-tuple-variant branch June 28, 2024 09:23
This was referenced Jun 28, 2024
Merged
Merged
@Leonidas-from-XIV Leonidas-from-XIV mentioned this pull request May 29, 2025
Merged
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request May 29, 2025
@Leonidas-from-XIV
[new release] yojson and yojson-five (3.0.0)
91e8051 
CHANGES:

*2025-05-39*

### Changed

- Floats are now always output to JSON in a standard-conformant way or not at
  all (raising an exception). This makes the `std` variants of functions
  identical to the non-`std` variants and the `std` arguments have no effect.
  Users are encouraged to switch to the non-`std` affixed variants, the others
  will be deprecated in the future.  (ocaml-community/yojson#184, @Leonidas-from-XIV)
- Bumped the minimum required version of OCaml for the main package to 4.08
  since the CI dropped the support. This however allows removing the dependency
  on the `seq` library, so the depencency cone becomes slightly smaller. (ocaml-community/yojson#194,
  @Leonidas-from-XIV)

### Fixed

- Fixed handling of escape sequences in JSON5. Known escapes like \b will be
  properly unescaped and undefined escape sequences will unescape to the
  character itself as per spec (ocaml-community/yojson#187, @david-maison-TrustInSoft)
- Fixed tests failing on Windows due to disagreements with the length of an
  input channel and the text mode conversion (ocaml-community/yojson#192, @Leonidas-from-XIV)

### Removed

- Removed support for Tuple and Variant in JSON. It was a non-standard
  extension that was rarely used, so this simplifies the Yojson types and the
  parser more standard-conforming (ocaml-community/yojson#105, ocaml-community/yojson#158, ocaml-community/yojson#185 @Leonidas-from-XIV)
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Jun 2, 2025
@Leonidas-from-XIV
[new release] yojson and yojson-five (3.0.0)
2e3ff1f 
CHANGES:

*2025-05-39*

### Changed

- Floats are now always output to JSON in a standard-conformant way or not at
  all (raising an exception). This makes the `std` variants of functions
  identical to the non-`std` variants and the `std` arguments have no effect.
  Users are encouraged to switch to the non-`std` affixed variants, the others
  will be deprecated in the future.  (ocaml-community/yojson#184, @Leonidas-from-XIV)
- Bumped the minimum required version of OCaml for the main package to 4.08
  since the CI dropped the support. This however allows removing the dependency
  on the `seq` library, so the depencency cone becomes slightly smaller. (ocaml-community/yojson#194,
  @Leonidas-from-XIV)

### Fixed

- Fixed handling of escape sequences in JSON5. Known escapes like \b will be
  properly unescaped and undefined escape sequences will unescape to the
  character itself as per spec (ocaml-community/yojson#187, @david-maison-TrustInSoft)
- Fixed tests failing on Windows due to disagreements with the length of an
  input channel and the text mode conversion (ocaml-community/yojson#192, @Leonidas-from-XIV)

### Removed

- Removed support for Tuple and Variant in JSON. It was a non-standard
  extension that was rarely used, so this simplifies the Yojson types and the
  parser more standard-conforming (ocaml-community/yojson#105, ocaml-community/yojson#158, ocaml-community/yojson#185 @Leonidas-from-XIV)
dkalinichenko-js pushed a commit to dkalinichenko-js/opam-repository that referenced this pull request Jun 10, 2025
@Leonidas-from-XIV @d-kalinichenko
[new release] yojson and yojson-five (3.0.0)
10cff74 
CHANGES:

*2025-05-39*

### Changed

- Floats are now always output to JSON in a standard-conformant way or not at
  all (raising an exception). This makes the `std` variants of functions
  identical to the non-`std` variants and the `std` arguments have no effect.
  Users are encouraged to switch to the non-`std` affixed variants, the others
  will be deprecated in the future.  (ocaml-community/yojson#184, @Leonidas-from-XIV)
- Bumped the minimum required version of OCaml for the main package to 4.08
  since the CI dropped the support. This however allows removing the dependency
  on the `seq` library, so the depencency cone becomes slightly smaller. (ocaml-community/yojson#194,
  @Leonidas-from-XIV)

### Fixed

- Fixed handling of escape sequences in JSON5. Known escapes like \b will be
  properly unescaped and undefined escape sequences will unescape to the
  character itself as per spec (ocaml-community/yojson#187, @david-maison-TrustInSoft)
- Fixed tests failing on Windows due to disagreements with the length of an
  input channel and the text mode conversion (ocaml-community/yojson#192, @Leonidas-from-XIV)

### Removed

- Removed support for Tuple and Variant in JSON. It was a non-standard
  extension that was rarely used, so this simplifies the Yojson types and the
  parser more standard-conforming (ocaml-community/yojson#105, ocaml-community/yojson#158, ocaml-community/yojson#185 @Leonidas-from-XIV)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hhugo hhugo hhugo left review comments

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

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

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

Successfully merging this pull request may close these issues.

[RFC] Remove support for Tuple and Variant
4 participants
@Leonidas-from-XIV @c-cube @mjambon @hhugo
0