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

Mdx_top.init can be parameterized by dirs, packages and predicates #244

Merged
merged 1 commit into from
Nov 16, 2020
Merged

Mdx_top.init can be parameterized by dirs, packages and predicates #244

merged 1 commit into from
Nov 16, 2020

Conversation

gpetiot
Copy link
Contributor

@gpetiot gpetiot commented Apr 3, 2020

@NathanReb let me know if this is what you had in mind, before I go further (if yes, I still have to re-architecture the lib to make the whole test function customizable by the dirs).

Copy link
Contributor

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most important part here the ~dirs argument as that's the only thing dune will need to provide since the libraries will already be linked in.

I think it makes sense to expose packages and predicates as arguments as well.
Obviously once we've extracted the test logic into a library, they should be arguments, of the entry point for running the tests.

bin/test/main.ml Outdated Show resolved Hide resolved
@gpetiot gpetiot marked this pull request as ready for review April 17, 2020 17:17
@gpetiot
Copy link
Contributor Author

gpetiot commented Apr 17, 2020

I defined a new library mdx.test, let me know if that's enough for dune.

@gpetiot gpetiot requested a review from NathanReb April 17, 2020 17:18
@gpetiot gpetiot added the no changelog This PR doesn't require a changelog update label Aug 3, 2020
@gpetiot
Copy link
Contributor Author

gpetiot commented Nov 13, 2020

I rebased it on master, @voodoos if there is everything you need for the mdx stanza for dune I think we should merge this one, as it's a bit painful to rebase when there is any change in bin/test/main.ml (which happens often)

@voodoos
Copy link
Collaborator

voodoos commented Nov 13, 2020

I think everything I need is here, as Nathan said, it is mostly the ~dir parameter which will be useful.

@NathanReb, do you see anything missing ?

Copy link
Contributor

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure you're right, let's merge this, we can always make changes later if it needs some adjustment. It is already a good change on its own to move the logic from the bin/test/main.ml to the library.

Thanks for working on this and sorry the rebase hassle!

@gpetiot gpetiot merged commit b056a89 into realworldocaml:master Nov 16, 2020
@gpetiot gpetiot deleted the parameterize-toplevel-env branch November 16, 2020 10:27
@voodoos voodoos mentioned this pull request Nov 18, 2020
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This PR doesn't require a changelog update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants