search

ocaml / stdlib-random

Versioned random number library
Other
6 stars 1 forks source link

Review library design before publication #1

Closed Octachron closed 1 year ago

dbuenzli commented 2 years ago

I had a quick look, it looks mostly good. I have two comments:

  1. What you propose works but I would however likely have opted for a different library structure with a single package (e.g. stdlib-random) that provides all the ocamlfind libraries (e.g. as stdlib-random.{v5,v5o,v4}).

  2. There was at least another change at some point in the PRNG that IIRC broke the generative art here http://www.random-art.org/. it may be worth adding the version prior to that. I suppose this was was 3.12, and this change set, maybe @andrejbauer can confirm.

Octachron commented 2 years ago

Thanks for the comments! I can definitively extend the compatibility to the PRNGs used between OCaml 3.07 and OCaml 3.12, and the one used before that.

I am not really sure what is the best option between having one or multiple opam packages in this case, if you have any criteria to make these kind of choice, I am all ears.

dbuenzli commented 2 years ago

if you have any criteria to make these kind of choice, I am all ears.

I'd say usually you start splitting along package lines when you have additional functionality that needs more dependencies that not all users may want to have in their dependency cone.

Since here all the libraries have the same dependency set (ocaml) it feels a bit unwaranted (other people could tell you oh but if I'm only using v5 I don't want to compile v4, but I don't think the build times are egregious here).

Also now that the number of versions is increasing you can see that it becomes more bureaucratic than it could be given the current state of the eco-system. For example if I want to be able to use all n versions in my program I need to specify n packages and n libraries instead of 1 package and n libraries.

dra27 commented 2 years ago

I certainly agree with one opam package. Might it even be worth going one stage further and having just the one library (and let module aliases do its thing?) - cf. mirage/checkseum for a mildly similar problem.

Octachron commented 2 years ago

Isn't the possibility to use a virtual module to let users choose the V5 implementation an argument to keep the implementation separated? At least, as far as I can see, the choice of implementation for the virtual module is done at the library level.

Octachron commented 2 years ago

I have switched to a single opam package and findlib library.

I have also added the PRNG used between from OCaml 3.07 to OCaml 3.12, but diving deeper requires more compromise between compatibility and correctness that I am willing to arbitrate without an existing use case in mind.

I have also renamed the package and library to stdlib-random since stdlib-random.v5 seemed like a sweet spot between conciseness and readability.

Thanks for the feedback!

dbuenzli commented 2 years ago

That looks good.

Just one last plea. There's quite a bit of churn between packages/subpackages/libraries in the README and the opam description.

I think it's good if we can follow the simpler terminology used by dune that opam distributes packages which are simply sets of libraries (technically: ocamlfind packages and subpackages).

In this case the opam package is stdlib-random and the libraries are stdlib-random.v[3-5]o?.

dbuenzli commented 2 years ago

Also for least surprise it would be good to rename the github repo to stdlib-random.

dbuenzli commented 2 years ago

Finally :-) it would be nice to rather have an index.mld file with what you wrote in the README. so that it shows up in the package landing page of ocaml.org and odig.

Octachron commented 2 years ago

Good point on keeping the terminology simple and uniform.

I will add an index.mld file once I am able to read the README without updating it.