-
Notifications
You must be signed in to change notification settings - Fork 10
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
Initiate ortac-example package #177
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
I’ve put a couple of suggestions as commits on my branch lwt-dllist-bis
(and on (n-osborne#21)).
Other suggestions:
- I would have preferred it if
ocamlformat
had been disabled onlwt_dllist_spec.mli
to make it easier to compare with the upstream version (it would also help maintain it) - I expected that one of the
take_*r
functions would have a specification readingmatch Sequence.length (old s.contents) with 0 -> ...
to illustrate that matching on integers is now supported :-) (it would also avoid duplicate calls toSequence.length
intake_opt_r
) - the dune rule to generate the
lwt_dllist_tests
would be nice there, even if only commented (to be honest, I would suggest to post-process them with a small script to inject the hand-written part :-)
Oh! last suggestion: we are in the ortac repository, so I would prefer the directory to be named |
Suggestions for ocaml-gospel#177
70aa2ce
to
9c3364c
Compare
9345b72
to
465a617
Compare
a08b48e
to
1575f0f
Compare
1575f0f
to
71d2805
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve reviewed the new changes: well done! LGTM!
The ortac-examples package aims at centralising specifications and ortac-generated code based on those specifications.
The first added example is the
lwt_dllist
librarie. This example display how to handle extension of theSTM.ty
type.While writing the specification, I've stumbled upon #171 and #172.