Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Upgrade to dune and Opam 2 #3

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

selasiehanson
Copy link

No description provided.

@selasiehanson selasiehanson changed the title Upgrade to dune to and Opam 2 Upgrade to dune and Opam 2 Oct 21, 2019
@selasiehanson
Copy link
Author

@andreas, I'm not sure if had a chance to look at this.

@andreas
Copy link
Owner

andreas commented Nov 3, 2019

Sorry for the late reply — for some reason I didn’t get an email notification about the PR. I’ll take a look this week.

Copy link
Owner

@andreas andreas left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I added a few comments.

dataloader-lwt.opam Outdated Show resolved Hide resolved
(deps
(:< test.exe))
(action
(run %{<} -v)))
Copy link
Owner

Choose a reason for hiding this comment

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

I think this file can be define more concisely like so:

(test
 (libraries dataloader-lwt lwt.unix alcotest)
 (name test))

dataloader.opam Outdated Show resolved Hide resolved
(deps
(:< test.exe))
(action
(run %{<} -v)))
Copy link
Owner

Choose a reason for hiding this comment

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

This file can be shortened like the other one 😄

dataloader.opam Outdated
depends: [
"jbuilder" {build}
"ocaml" {>= "4.03.0"}
"dune" {build & >= "1.0"}
"rresult"
Copy link
Owner

Choose a reason for hiding this comment

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

It turns out rresult is not used, so would be great to remove.

(name dataloader)
(public_name dataloader)
(wrapped false)
(libraries result))
Copy link
Owner

Choose a reason for hiding this comment

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

result can be removed here 😄

selasiehanson and others added 6 commits November 24, 2019 20:49
annotate alcotest with `{with-test}` variable

Co-Authored-By: Andreas Garnaes <andreas@users.noreply.github.com>
test annotation for alcotest

Co-Authored-By: Andreas Garnaes <andreas@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants