ocaml-decoders conversion experiment #1280
Conversation
| let decode = | ||
| Json.Decode.( | ||
| field("command", string) >>= command => | ||
| field("title", LocalizedToken.decode) >>= title => | ||
| field_opt("category", string) >>= category => | ||
| succeed({command, title, category}) | ||
| ); |
There was a problem hiding this comment.
Example of decoder using field and monadic bind (>>=). Not too bad for small decoders.
src/Extensions/ExtensionManifest.re
Outdated
| let manifest = | ||
| field("name", string) >>= name => | ||
| field("version", string) >>= version => | ||
| one_of([ | ||
| ("author", field("author", author)), | ||
| ("publisher", field("publisher", string)), | ||
| ("default", succeed("Unknown Author")), | ||
| ]) >>= author => | ||
| field_opt("displayName", LocalizedToken.decode) >>= displayName => | ||
| field_opt("description", string) >>= description => | ||
| field_opt("main", string) >>= main => | ||
| field_opt("icon", string) >>= icon => | ||
| field_opt("categories", list(string)) |> default([]) >>= categories => | ||
| field_opt("keywords", list(string)) |> default([]) >>= keywords => | ||
| field("engines", engine) >>= engines => | ||
| field_opt("activationEvents", list(string)) |> default([]) >>= activationEvents => | ||
| field_opt("extensionDependencies", list(string)) |> default([]) >>= extensionDependencies => | ||
| field_opt("extensionPack", list(string)) |> default([]) >>= extensionPack => | ||
| field_opt("extensionKind", kind) |> default(Ui) >>= extensionKind => | ||
| field("contributes", ExtensionContributions.decode) >>= contributes => | ||
| field_opt("enableProposedApi", bool) |> default(false) >>= enableProposedApi => | ||
| succeed({ | ||
| name, version, author, displayName, description, main, icon, | ||
| categories, keywords, engines, activationEvents, extensionDependencies, | ||
| extensionPack, extensionKind, contributes, enableProposedApi | ||
| }); |
There was a problem hiding this comment.
Significantly less readable for large decoders.
Note that there's also a "pipeline" API which is more readable, and can reduce the boilerplate a bit with generated record constructors, but it depends on the position of arguments and is therefore more error-prone, and also yields harder-to understand errors messages.
src/Extensions/ExtensionManifest.re
Outdated
| field_opt("displayName", LocalizedToken.decode) | ||
| >>= ( | ||
| displayName => | ||
| field_opt("description", string) | ||
| >>= ( | ||
| description => | ||
| field_opt("main", string) | ||
| >>= ( | ||
| main => | ||
| field_opt("icon", string) | ||
| >>= ( | ||
| icon => | ||
| field_opt("categories", list(string)) | ||
| |> default([]) | ||
| >>= ( | ||
| categories => | ||
| field_opt("keywords", list(string)) | ||
| |> default([]) | ||
| >>= ( | ||
| keywords => | ||
| field("engines", engine) | ||
| >>= ( | ||
| engines => | ||
| field_opt( | ||
| "activationEvents", | ||
| list(string), | ||
| ) | ||
| |> default([]) | ||
| >>= ( | ||
| activationEvents => | ||
| field_opt( | ||
| "extensionDependencies", | ||
| list(string), | ||
| ) | ||
| |> default([]) | ||
| >>= ( | ||
| extensionDependencies => | ||
| field_opt( | ||
| "extensionPack", | ||
| list(string), | ||
| ) | ||
| |> default([]) | ||
| >>= ( | ||
| extensionPack => | ||
| field_opt( | ||
| "extensionKind", | ||
| kind, | ||
| ) | ||
| |> default(Ui) | ||
| >>= ( | ||
| extensionKind => | ||
| field( | ||
| "contributes", | ||
| ExtensionContributions.decode, | ||
| ) | ||
| >>= ( | ||
| contributes => | ||
| field_opt( | ||
| "enableProposedApi", | ||
| bool, | ||
| ) | ||
| |> default( | ||
| false, | ||
| ) | ||
| >>= ( | ||
| enableProposedApi => | ||
| succeed({ | ||
| name, | ||
| version, | ||
| author, | ||
| displayName, | ||
| description, | ||
| main, | ||
| icon, | ||
| categories, | ||
| keywords, | ||
| engines, | ||
| activationEvents, | ||
| extensionDependencies, | ||
| extensionPack, | ||
| extensionKind, | ||
| contributes, | ||
| enableProposedApi, | ||
| }) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Oh dear god refmt. Wat u doin?
| let decode = | ||
| Json.Decode.( | ||
| field("command", string) | ||
| >>= ( | ||
| command => | ||
| field("title", LocalizedToken.decode) | ||
| >>= ( | ||
| title => | ||
| field_opt("category", string) | ||
| >>= (category => succeed({command, title, category})) | ||
| ) | ||
| obj(({field, _}) => | ||
| { | ||
| command: field.required("command", string), | ||
| title: field.required("title", LocalizedToken.decode), | ||
| category: field.optional("category", string), | ||
| } | ||
| ) | ||
| ); |
There was a problem hiding this comment.
An example of a small decoder using obj
| ("publisher", field.monadic("publisher", string)), | ||
| ("default", succeed("Unknown Author")), | ||
| ]), | ||
| ), |
There was a problem hiding this comment.
This is one of the shortcomings of the obj decoder. Because the field.* decoders aren't "normal" monadic decoders, they can't be composed. So we need an escape hatch. This is it, called whatever while I think of a better name for it.
| enableProposedApi: | ||
| field.withDefault("enableProposedApi", false, bool), | ||
| } | ||
| ) |
There was a problem hiding this comment.
Otherwise I think this looks significantly better than the alternatives.
| @@ -32,6 +32,7 @@ This project incorporates components from the projects listed below. The origina | |||
| 25. fmt (https://github.com/dbuenzli/fmt) | |||
| 26. LaserWave (https://github.com/Jaredk3nt/laserwave) | |||
| 27. camlp5 (https://github.com/camlp5/camlp5) | |||
| 28. ocaml-decoders (https://github.com/mattjbray/ocaml-decoders) | |||
There was a problem hiding this comment.
Added ocaml-decoders, but the license is just ISC. Literally, that is the entire license text. So should I just add that to the end here, find the text of the ISC license from somewhere else, or just leave it as-is?
There was a problem hiding this comment.
This in particular ought to be addressed before merging @bryphe
There was a problem hiding this comment.
Oh, thanks for the ping @glennsl ! Definitely - sorry I missed this.
Let's just add the ISC license text (from here: https://opensource.org/licenses/ISC)
Copyright 2020 mattjbray
Permission to use, copy, modify, and/or distribute this software for any purpose with or without fee is hereby granted, provided that the above copyright notice and this permission notice appear in all copies.
THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
Thanks for checking in on this!
This converts most notably
ExtensionManifest,ExtensionContributionsandExtHostInitData, but alsoConfigurationandLocalizedToken, to useocaml-decodersinstead ofppx_derigin_yojsonfor encoding and decoding of json.Currently this adds quite a bit of extra code. That's partly because of the infrastructure additions in
Core.Jsonwhich required copying parts ofocaml-decodersto get access to some internals. That shouldn't really be necessary, and is a one-time cost either way. But more on that below.A lot of the remaining weight is because this just mechanically reimplements the decoders and encoders generated by
ppx_deriving_yojson. But a lot of this, and existing code, is unnecessary because, with custom decoders, intermediate data structured are no longer necessary just to generate the decoders. That is the point of this after all, to decode and encode directly to and from application data structures and save the intermediate data structure and extra conversions.Unfortunately these intermediate data structures reach far into the model and beyond, and will take some work to untangle. But it does also serve to illustrate the point, that when intermediate data structures are used for the purpose of "convenient" generation fo json encoders and decoders, they tend to end up being used where they shouldn't, in place of application-specific data structures serving application needs, and making a mess of dependencies.
As for using the decoders themselves, I recommend stepping through the commits to see the progression. Decoding objects with the
fielddecoder and monadic bind operator was a pretty messy affair, and helped byrefmtstill not having a clue about the monadic bind operator and aiming for the pyramid of doom instead. This could be improved with the monadic let bindings in 4.08, but I've also added a customobjdecoder that I think provides a significantly better experience that that would anyway. It's not without shortcomings though - in particular because the provided decoders can't be composed as normal decoders - and the API should still be considered unfinished. It also required copying parts of theocaml-decodersimplementation to get at the internals. We should try to get this upstreamed in some way to avoid that.Caveats aside, I hope this is sufficient to get a good idea of how
ocaml0decoderswork and how the two approaches compare. Whereocaml-decodersreplace custom yojson encoders and decoders, the former is certainly much nicer to use. I also like to have the decoding and encoding separate from the data structure. But most important I think is that it removes the need for intermediate data structures, and the temptation to use them where they shouldn't, because if you don't you often end up with just as much conversion code, but now have the overhead of conversion in addition to the hidden complexity of decoding and encoding.