mirage / ocaml-github

GitHub APIv3 OCaml bindings
ISC License
100 stars 61 forks source link

Move from atdgen to ppx_deriving_yojson #109

Open rgrinberg opened 8 years ago

rgrinberg commented 8 years ago

Or to jsont if it ever gets released. My main motivation for this is to add the common deriving converters to the type definitions like sexp.

dsheets commented 8 years ago

We need support for something like atdgen's tag_field which is request in https://github.com/whitequark/ppx_deriving_yojson/issues/27 and looked like it was rejected for a time.

If we want to use jsont, we need a staging solution.

rgrinberg commented 8 years ago

This may be useful as a transitional https://github.com/mjambon/atdgen/pull/44 tool

dbuenzli commented 8 years ago

Or to jsont if it ever gets released.

It will be.

If we want to use jsont, we need a staging solution.

@dsheets What do you mean by this ?

FWIW I'm interacting with GitHub's API at the moment but it's out of question to have the insane dependency cone of ocaml-github (see the daunting output of opam list --rec --required-by github) in my software. Especially I do not use anything that breaks with every major OCaml release as it's a waste of everybody's time (most people using ppx only focus on the very little boiler plate they save but don't realize the huge maintenance and annoyance cost it has; if only people knew more about type indexed combinators...).

So I think it would be nice to have a base library (maybe even as a separate project) that simply has abstract data types and jsont codecs that models the API. This would allow to reuse the definitions with which ever transport layer you'd like --- in my case, the curl command line tool.

avsm commented 8 years ago

On 12 Apr 2016, at 11:48, Daniel Bünzli notifications@github.com wrote:

So I think it would be nice to have a base library (maybe even as a separate project) that simply has abstract data types and jsont codecs that models the API. This would allow to reuse the definitions with which ever transport layer you'd like --- in my case, the curl command line tool.

Yes, this is my preferred solution also. A dependency-free (aside from jsont) library of the API itself would be nice to have.

rgrinberg commented 8 years ago

This would not really satisfy me as a user. Sexp converters are a very useful debugging tool for me with concrete benefits. The fruits of a well pruned dependency tree on the other hand are harder to imagine.

A dependency-free (aside from jsont) library of the API itself would be nice to have.

Why stop there? Let's make a library with just the types and jsont can be added on top (in a separate project too!). I'm sure we can find someone out there who will complain about that dependency too.

avsm commented 8 years ago

this doesn't seem to be incompatible with having sexps -- I'm just thinking of replacing the atd description of the API with jsont and making that a separate package. We could do the same thing for just the ATD spec (in a similar manner to wire format descriptions in cstruct/mirage).

I think it's possible to do this in a single repo these days by using an opam.<pkg>/ directory in the root, but I've not tried that. I dont think splitting up the repo is a good thing to do, but I am strongly in favour of mapping ocamlfind subpackages 1:1 to opam package names where possible, and removing depopts

dbuenzli commented 8 years ago

Sexp converters are a very useful debugging tool for me with concrete benefits.

If debugging is the only reason, formatters can do the job. If you really insist on sexps then you don't need to generate them, a codec function for a datatype should be only a few typed indexed combinators away.

The fruits of a well pruned dependency tree on the other hand are harder to imagine.

I hope you had a look at the result of the invocation I mentioned earlier. I'm not sure why I have to depend on shared-memory-ring if I want to interact with github using curl the tool or curl the library. In my book this is called an engineering disaster. What we have here a library that doesn't compose in a graceful way (if not at all) with different usage scenarios.

But then maybe you like bloat.

Why stop there? Let's make a library with just the types and jsont can be added on top (in a separate project too!). I'm sure we can find someone out there who will complain about that dependency too.

Such a person would exercise very poor taste... As always good design is about finding the right balance, this applies to your dependency cone too.

avsm commented 8 years ago

I'm not sure why I have to depend on shared-memory-ring if I want to interact with github using curl the tool or curl the library. In my book this is called an engineering disaster.

In my book, that is called a bug, and can be fixed like one if you create an issue about it. We just need to rearrange the opam packages so that they follow the same dependency cone as the modules contained within the various ocamlfind packages. At a guess, your OPAM invocation is probably resolving depopts as well with the --rec (and so pulling in the full suite of conduit dependencies). shared-memory-ring is not actually installed if the user does opam install github:

  - install conf-m4         1        
  - install conf-pkg-config 1.0      
  - install conf-openssl    1        
  - install conf-ncurses    1        
  - install ocamlfind       1.6.2    
  - install type_conv       113.00.02
  - install ssl             0.5.2    
  - install sexplib         113.33.00
  - install react           1.2.0    
  - install ppx_tools       4.02.3   
  - install menhir          20160303 
  - install magic-mime      1.0.0    
  - install fieldslib       113.24.00
  - install easy-format     1.2.0    
  - install cppo            1.3.1    
  - install camomile        0.8.5    
  - install base-bytes      base     
  - install pa_sexp_conv    113.00.01
  - install ppx_core        113.33.00
  - install lwt             2.5.1    
  - install biniou          1.0.9    
  - install atd             1.1.2    
  - install ppx_deriving    3.3      
  - install zed             1.4      
  - install stringext       1.4.2    
  - install re              1.5.0    
  - install ocplib-endian   0.8      
  - install base64          2.0.0    
  - install ppx_optcomp     113.33.00
  - install yojson          1.3.2    
  - install lambda-term     1.10     
  - install cstruct         1.9.0    
  - install ppx_driver      113.33.00
  - install atdgen          1.9.0    
  - install ppx_type_conv   113.33.00
  - install ppx_sexp_conv   113.33.00
  - install ppx_fields_conv 113.33.00
  - install uri             1.9.2    
  - install ipaddr          2.7.0    
  - install conduit         0.11.0   
  - install cohttp          0.20.2   

there are a few things that could be trimmed from this list (e.g. magic-mime is only really for cohttp server, and so not necessary for a client package), but overall a lot of it is ppx build time stuff. Not exactly an engineering disaster...

dbuenzli commented 8 years ago

Not exactly an engineering disaster...

Can I re-use ocaml-github's codecs in the browser with js_of_ocaml ? Given the list you output, I have my doubts. Sorry, I just have a different notion of what writing composable software means.

rgrinberg commented 8 years ago

If debugging is the only reason, formatters can do the job. If you really insist on sexps then you don't need to generate them, a codec function for a datatype should be only a few typed indexed combinators away.

So we can at least agree that sexps solve real problems.

Formatters are fine for debugging too, because I can also generate them.

I hope you had a look at the result of the invocation I mentioned earlier. I'm not sure why I have to depend on shared-memory-ring if I want to interact with github using curl the tool or curl the library. In my book this is called an engineering disaster. What we have here a library that doesn't compose in a graceful way (if not at all) with different usage scenarios.

But then maybe you like bloat.

No, I agree with this point. ocaml-github can certainly structure the dependencies better. But I distinguish between different dependencies. Some like you've pointed out are overly specific and require users to pay in portability, binary bloat, external dependencies, etc. I just don't think sexp is one such dependency. I guess I still don't understand your objections against ppx?!

@avsm how did lambda-term make that list?

rgrinberg commented 8 years ago

Can I re-use ocaml-github's codecs in the browser with js_of_ocaml ? Given the list you output, I have my doubts. Sorry, I just have a different notion of what writing composable software means.

Maybe if jsont was released that would be done already, it's not really a question of dependencies. I'd much rather work on that on dependency minimization crusades that I will have to undo in my own software anyway.

avsm commented 8 years ago

Can I re-use ocaml-github's codecs in the browser with js_of_ocaml ?

https://github.com/mirage/ocaml-github/tree/master/js

Given the list you output, I have my doubts. Sorry, I just have a different notion of what writing composable software means.

To reiterate: the opam package list does not reflect the actual OCaml dependencies for a particular slice of functionality until we harmonise the state of opam vs ocamlfind vs oasis. For a JavaScript backend, the functor application is:

module Time = struct
  let now = Unix.gettimeofday
  let sleep = Lwt_js.sleep
end

module Github' = Github_core.Make(Time)(Cohttp_lwt_xhr.Client)
include Github'
dbuenzli commented 8 years ago

Don't have to write any code to pretty print for debugging. "A few typed indexed combinators away" is still an annoying distraction to me when I'm debugging

These of course should be written by the library author.

I guess I still don't understand your objections against ppx?!

There are many (I won't talk about the lack of compositionality), but from an eco-system perspective, I don't think people fully grasp the hidden costs of ppxs, which trickle down to the OCaml system itself. I bet the 4.03 betas didn't get as much testing as needed because many things could simply not compile there because they were using ppxs.

I personally had to stop testing it because even the bloody utop thing didn't work there (because of lwt's ppx). Let me reiterate this: any software piece that has a ppx in his dependency cone is potentially not able to make it through a major OCaml release gracefully. The costs in build failures, build fixes, software fixes, upgrade to the package's metadata, frustration by the users, inability to test or evaluate designs properly (especially for the core dev team) are huge. How much of my packages needed a fix to be able to install in a 4.03 switch ? None, and that's the way it should be if you want to be able to scale modularity.