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

Split Lin package #164

Merged
merged 6 commits into from
Nov 16, 2022
Merged

Split Lin package #164

merged 6 commits into from
Nov 16, 2022

Conversation

shym
Copy link
Collaborator

@shym shym commented Oct 21, 2022

Refactor the Lin package into sub-libraries so that users can choose to use only the domain, the thread or the effect implementation. That way it is possible to test the domain and effect implementation in runs without running first the thread initialisation code.
Put the part of the interface that should not be used by external users into _internal modules. They can’t be really hidden (by using dune private_modules stanza for instance) without breaking the test suite.

This PR is really only about the couple of commits at the tip of that branch. They are currently based off an integration branch because it is meant to be merged when #112 will have been merged.

@shym shym marked this pull request as draft October 21, 2022 17:49
@jmid jmid added this to the First opam release milestone Oct 24, 2022
@jmid
Copy link
Collaborator

jmid commented Nov 11, 2022

I've now taken a pass over this PR.
Overall I was very pleased to see how systematic it approaches the refactoring.
It made reading the diff commit-by-commit a pleasure! 😃

A few things I noticed - only related to naming really:

  • Why change ApiSpec to Spec? I think the former is more descriptive. InterfaceSpec may be more in line with established OCaml terminology though...
  • I'm not sure about the ExpandedSpec name as it is not very descriptive: such modules contain generators, printers, shrinkers, etc., so they are further from a 'specification'. I think of this old Lin as an internal test representation myself. Could Lin_internal.TestRepr and Lin_common.Make/Build_test_repr work? (other name suggestions welcome!)
  • Finally the old "Lin_api" and "Lin" test name prefixes should probably be updated in a separate commit of the PR

@shym shym force-pushed the split-lin-packages branch from 6a78c51 to 4c9b002 Compare November 16, 2022 10:38
@shym
Copy link
Collaborator Author

shym commented Nov 16, 2022

I rebased (well, largely rewritten) this PR on main.
I hope it is also a lot more thorough, thanks to #188: a few tests were not updated to the new interface.

I do agree with your naming remarks, so I did revert the changes I had made. I suggest we do rename (to InterfaceSpec and Make_test_repr for instance) in a further PR.
About the test name prefixes, I went with Lin DSL and left Lin intact. My reasoning was that the nuance is really only for us to know which test was really run, something like Lin internal would just add noise.

Last point that requires work (maybe in a further PR also?) is src/kcas that I left completely untouched since I don’t know whether that code is still alive (I should probably install that library to test changes I would do there if we want to update it).

@shym shym marked this pull request as ready for review November 16, 2022 10:50
@shym
Copy link
Collaborator Author

shym commented Nov 16, 2022

It also does not yet update the documentation, but I suggest we do this also in a separate PR. Since this one is modifying so many source files in our test suite, we would avoid a lot of conflicts by merging soon.

@shym shym force-pushed the split-lin-packages branch from 4c9b002 to 45f27f0 Compare November 16, 2022 16:29
shym added 6 commits November 16, 2022 17:50
This commit only moves lines into their respective new modules so that
it is possible to check that it does no other refactoring. Thus it
obviously does not build

Bisection: skip
Add a Lin_base module, as the entrypoint of the qcheck-lin.base library
@shym shym force-pushed the split-lin-packages branch from 45f27f0 to fd1e200 Compare November 16, 2022 17:30
@shym
Copy link
Collaborator Author

shym commented Nov 16, 2022

And rebased on main.

jmid added a commit that referenced this pull request Nov 16, 2022
@jmid jmid merged commit d7d2e1e into ocaml-multicore:main Nov 16, 2022
@jmid
Copy link
Collaborator

jmid commented Nov 16, 2022

Thanks a bunch for this 🙏
I went over the commits again, found a small nit, committed a fix, and merged into main

@jmid jmid mentioned this pull request Nov 16, 2022
@shym shym mentioned this pull request Nov 22, 2022
@shym shym deleted the split-lin-packages branch March 20, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants