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

Update OCaml snippets #346

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

Update OCaml snippets #346

wants to merge 27 commits into from

Conversation

ComanderP
Copy link

As discussed in #345, I've updated most of the OCaml snippets to be simpler, not depend on anything besides the standard library, and also fixed mistakes. I've also changed some formatting choices for consistency throughout the book.

I built the book locally and it seemed fine, hopefully everything's alright.

@hmemcpy
Copy link
Owner

hmemcpy commented Jan 3, 2025

Thank you so much for this MASSIVE effort!
I've asked on twitter for the OCaml crew to help review this. It all looks good to me, but unfortunately I don't speak OCaml ;)

Let's wait for a few LGTMs.

And thank you again!

Copy link

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

LGTM!

Impressive amount of work! I haven't looked closely through everything but it looks reasonable enough after the first look.

@yawaramin
Copy link

Hi, I'm doing a review. Will get back soon.

Copy link

@yawaramin yawaramin left a comment

Choose a reason for hiding this comment

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

Sending what I have so far :-)

src/content/1.10/code/ocaml/snippet05.ml Show resolved Hide resolved
src/content/1.10/code/ocaml/snippet06.ml Show resolved Hide resolved
@@ -1,2 +1,4 @@
(* Starting with empty list *)
let fmap f (safe_head []) = fmap f None = None
(* As a reminder, this is not actual code *)

Choose a reason for hiding this comment

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

To clarify, I would say something like: 'As a reminder, this is not actual OCaml code, it is a demonstration of equational reasoning'.

Copy link
Author

Choose a reason for hiding this comment

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

Honestly, there are a lot of snippets that aren't really code (just basically equations) and the Haskell version would suffice in those cases. Maybe these could be removed instead of duplicating the equational reasoning?

src/content/1.10/code/ocaml/snippet10.ml Show resolved Hide resolved
src/content/1.10/code/ocaml/snippet11.ml Show resolved Hide resolved
src/content/1.2/code/ocaml/snippet04.ml Show resolved Hide resolved
@@ -1,8 +1,4 @@
module Chapter5_Product_Example :
Chapter5_Product
with type a = int

Choose a reason for hiding this comment

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

Why removed?

Copy link
Author

Choose a reason for hiding this comment

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

The with operator isn't adding much to this chapter example, I think. It's not like in the case of the Functor, Monad, Monoid, etc. signatures where you'd like to be able to call your implementation outside of the module itself, for example. Here, we're just constraining the types inside this module for the sake of the book's example.

src/content/1.6/code/ocaml/snippet12.ml Show resolved Hide resolved
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