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.

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

`of_json` #3776

Merged
merged 20 commits into from Mar 12, 2019

Conversation

Projects
None yet
4 participants
@gasche
Copy link
Member

commented Mar 10, 2019

This branch adds of_json deserializers to a lot of internal OPAM datatypes. The idea is for opam-lib users to be able to exchange data safely (@Armael and I are playing with computing list of co-installable packages at the MirageOS retreat).

The second half of the pull-request is a set of Crowbar generators for the internal data structures, checking that the serializers/deserializers roundtrip correctly.

One thing not implemented in the PR is an OpamJson.of_string function to decode JSON. I guess that, for this, it would be nicer to add a dependency to an existing JSON parser. (Edit: actually we don't need a JSON decoder in OPAM, see below.)

@gasche

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2019

@Armael just pointed out that we don't actually need to add any JSON-decoding logic in OPAM for our use-case: users of opam-lib can just link to their favorite JSON library that uses the same polymorphic variants, and everything will work fine. Yay.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2019

(Thanks @yomimono for merging stedolan/crowbar#47 which made my life easier.)

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2019

One thing not implemented in the PR is an OpamJson.of_string function to decode JSON. I guess that, for this, it would be nicer to add a dependency to an existing JSON parser.

That was the case in the past, see 1663fe0.

It's better if tools like opam minimize their their dependencies otherwise we quickly run into bootstrapping issues; this both for the dev tools the dependencies themselves may want to use and sometimes things we don't see but downstream system packagers actually run into.

@gasche gasche force-pushed the gasche:of_json_master branch from 87946b5 to b96bc46 Mar 11, 2019

Makefile targets instead of opam-crowbar.opam, neutralized into src/opam
This suggestion from Raja has the effect that `opam install .` at the
root does not draw in crowbar dependencies anymore. Instead, users can
do

    opam install src/crowbar --deps-only

and have the dependencies installed. (The file does not install
anything, it's just there for the dependencies.)
@gasche

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

I made some changes suggested by @rjbou : the .opam file for opam-crowbar is only in the src/crowbar subdirectory to make sure that people using opam install . at the root do not draw in the additional dependencies.

P.S.: and there are crowbar and crowbar-afl targets in the root Makefile to run the Crowbar tests more conveniently.

@AltGr

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Great, thanks !

@AltGr AltGr merged commit d16802d into ocaml:master Mar 12, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rjbou rjbou added this to the 2.1.0 milestone Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.