-
Notifications
You must be signed in to change notification settings - Fork 630
Update OCaml snippets #346
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
base: master
Are you sure you want to change the base?
Conversation
|
Thank you so much for this MASSIVE effort! Let's wait for a few LGTMs. And thank you again! |
chshersh
left a comment
There was a problem hiding this 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.
|
Hi, I'm doing a review. Will get back soon. |
yawaramin
left a comment
There was a problem hiding this 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 :-)
| @@ -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 *) | |||
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
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?
| @@ -1,8 +1,4 @@ | |||
| module Chapter5_Product_Example : | |||
| Chapter5_Product | |||
| with type a = int | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why removed?
There was a problem hiding this comment.
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.
|
Hi! Is there any progress on this? This is necessary for ocaml/ocaml.org#2975 |
|
On my side, I don't have enough knowledge in OCaml to review this. Perhaps @hmemcpy or @BartoszMilewski ? |
I've been waiting for more feedback, so that we can merge this PR. If anyone has an opinion on the previous unresolved conversations, and other chapter that haven't been reviewed, I'd appreciate it! |
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.