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

Initiate ortac-example package #177

Merged
merged 3 commits into from
Nov 22, 2023
Merged

Conversation

n-osborne
Copy link
Collaborator

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 the STM.ty type.

While writing the specification, I've stumbled upon #171 and #172.

Copy link
Contributor

@shym shym left a 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 on lwt_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 reading match Sequence.length (old s.contents) with 0 -> ... to illustrate that matching on integers is now supported :-) (it would also avoid duplicate calls to Sequence.length in take_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 :-)

@shym
Copy link
Contributor

shym commented Nov 10, 2023

Oh! last suggestion: we are in the ortac repository, so I would prefer the directory to be named examples, I feel annoyed by the redundancy :-)

n-osborne added a commit to n-osborne/ortac that referenced this pull request Nov 13, 2023
@n-osborne n-osborne force-pushed the lwt-dllist branch 5 times, most recently from 9345b72 to 465a617 Compare November 20, 2023 12:14
@n-osborne n-osborne force-pushed the lwt-dllist branch 2 times, most recently from a08b48e to 1575f0f Compare November 21, 2023 09:39
Copy link
Contributor

@shym shym left a 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!

@n-osborne n-osborne merged commit 0b4ec04 into ocaml-gospel:main Nov 22, 2023
2 of 3 checks passed
@n-osborne n-osborne deleted the lwt-dllist branch November 22, 2023 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants