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

add implementation stubs (fixes #285, fixes #298) #306

Merged
merged 6 commits into from
Jul 30, 2019

Conversation

marionebl
Copy link
Contributor

@marionebl marionebl commented Jun 14, 2019

@marionebl
Copy link
Contributor Author

CI failure appears to be unrelated.

@marionebl marionebl mentioned this pull request Jun 15, 2019
@@ -0,0 +1,15 @@
open Base

module Int_map = Map.M(Int)
Copy link
Contributor Author

@marionebl marionebl Jun 15, 2019

Choose a reason for hiding this comment

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

Addresses #298

@marionebl
Copy link
Contributor Author

marionebl commented Jun 15, 2019

As a side note: I dug into the CI setup a bit and created this version using CircleCI:
https://github.com/marionebl/ocaml/tree/circleci-setup

Maybe this also of interest for another PR?

@sshine sshine changed the title Add implementation stubs Add implementation stubs (fixes #285, fixes #298) Jun 19, 2019
@sshine
Copy link
Contributor

sshine commented Jun 21, 2019

Closing and opening PR to trigger CI. (I wonder if this is the best way.)

@sshine sshine closed this Jun 21, 2019
@sshine sshine reopened this Jun 21, 2019
@marionebl marionebl changed the title Add implementation stubs (fixes #285, fixes #298) WIP: Add implementation stubs (fixes #285, fixes #298) Jun 21, 2019
@sshine
Copy link
Contributor

sshine commented Jun 25, 2019

@marionebl: Could you merge master onto this feature branch?

I'm a little confused about the merge conflict; not sure if it's caused by leaving stuff out by intent, or because of breaking changes with base 0.12, or what.

Edit: Also, is this PR intended to be merged? I don't mind that stubs are added for a bunch of exercises at once, since the histories for those files start at nothing.

@marionebl
Copy link
Contributor Author

@marionebl: Could you merge master onto this feature branch?

I'll get this into a mergable state as soon as the spin offs for #311 land

@marionebl marionebl changed the title WIP: Add implementation stubs (fixes #285, fixes #298) add implementation stubs (fixes #285, fixes #298) Jun 28, 2019
@marionebl
Copy link
Contributor Author

marionebl commented Jun 28, 2019

One thing I don't like about this is the fact we do not have an automated test for the stub files as I did not find a way to build the tests without executing them.


Edit: One way to address this might be adding a new alias to the dune files

(alias
  (name    buildtest)
  (deps    (:x test.exe)))

Executing dune build @buildtest then builds all test without executing them, revealing issues with interfaces and implemenations stubs even before running tests.

@sshine sshine merged commit 6971e92 into exercism:master Jul 30, 2019
@sshine
Copy link
Contributor

sshine commented Jul 30, 2019

@marionebl: My model track is Haskell's, and here we run all tests of all example solutions on every CI run. Come to think of it, this is somewhat wasteful. I don't really know of the particular implications of adding such a build target alias -- my practical experience with OCaml build tools are very limited -- but I like the purpose very much.

If you're interested in becoming a co-maintainer for the OCaml track, you are very welcome.

If you prefer to submit changes as you have been doing so far, you are also very welcome.

@marionebl
Copy link
Contributor Author

marionebl commented Jul 31, 2019

If you're interested in becoming a co-maintainer for the OCaml track, you are very welcome.

I'd be glad to!

If you prefer to submit changes as you have been doing so far, you are also very welcome.

I'd continue submitting PRs even as maintainer so everyone stays on the same page.

@sshine
Copy link
Contributor

sshine commented Aug 1, 2019

@marionebl: Yes, that'd be ideal as long as we're more than one active maintainer, since we get the luxury of fault detection. (I don't think I can supplement your knowledge of CI and OCaml build tools, but four eyes are better than two!)

@iHiD, @kytrinyx:

  • Can we make @marionebl co-maintainer of exercism/ocaml?
  • And can we disable direct pushes to master? It works like this on exercism/haskell, so that all changes must come through a PR. (I'm not asking for required reviews, but rather just that we force the use of PRs, since they trigger CI.)

@iHiD
Copy link
Member

iHiD commented Aug 1, 2019

@kytrinyx will sort these for you.

@iHiD
Copy link
Member

iHiD commented Aug 1, 2019

@marionebl Welcome. Ping me on Slack and I'll add you to the #maintainers channel :)

@kytrinyx
Copy link
Member

kytrinyx commented Aug 1, 2019

@marionebl Welcome! Thank you so much for contributing ✨

I've sent the invitation to join the exercism/ocaml team, and have turned on branch protection for the master branch. Let me know if anything is wonky, I always worry that I get things wrong when messing with repo settings.

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.

4 participants