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

Improve benchmarks #12

Merged
merged 5 commits into from
Apr 21, 2015
Merged

Improve benchmarks #12

merged 5 commits into from
Apr 21, 2015

Conversation

smondet
Copy link
Owner

@smondet smondet commented Apr 16, 2015

Review on Reviewable

@rleonid
Copy link
Collaborator

rleonid commented Apr 21, 2015

Seems fine.

  1. At 650 lines of code, I would have separated main.ml before adding new logic.
  2. Writing your own Test framework makes it harder to grok. Took me a while to get used to check being an named assert.
  3. I'm not certain that this code needs to be so monadic since about half of your binds are just returning unit.
  4. debug_mode is always commented out, so what's the point of defining it?

@smondet
Copy link
Owner Author

smondet commented Apr 21, 2015

  1. At 650 lines of code, I would have separated main.ml before adding new logic.

OK will split: #14

  1. Writing your own Test framework makes it harder to grok. Took me a while to
    get used to check being an named assert.

That's a far more general problem :)

Finding a test frameworkd that doesn't suck more than 30 lines hacked together;
I haven't (in any language BTW).

The word assert has a very specific meaning in ocaml (evan emacs colors it
differently). Test.check seems OK for now and stresses the fact that it's just
a function (no compiler / syntax support).

  1. I'm not certain that this code needs to be so monadic since about half of
    your binds are just returning unit.

Those functions return (unit, 'some-error-type) Result.t Lwt.t; that's
much more than unit and we cannot escape it (that's how Lwt itself works).

  1. debug_mode is always commented out, so what's the point of defining it?

Well, for debugging :)
(I've used them a lot lately)

@smondet smondet merged commit 5a75664 into master Apr 21, 2015
smondet added a commit that referenced this pull request Apr 21, 2015
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