-
Notifications
You must be signed in to change notification settings - Fork 367
Improve Error Handling Documentation #950
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
Conversation
| exception Failure of string | ||
| ``` | ||
|
|
||
| * `Exit` terminates your program with a success status, which is 0 in Unices (they do error values) |
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.
"they do error values" is confusing here. Did you mean to mention that Unix-like systems have processes returning an exit value? Does this even matter in this tutorial?
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.
In the first section, I describe encoding errors as special values, as done in C. Mainly to state explicitly it shouldn't be done in OCAml. The text between the parenthesis is not a strict requirement for this tutorial, however, I'm tempted to believe it has some relevance. But I'm open to discussion. I agree the text needs to be clarified.
|
|
||
| Properly handling errors is a complex matter. It is [cross-cutting | ||
| concern](https://en.wikipedia.org/wiki/Cross-cutting_concern), it touches all | ||
| parts of an application and can't be isolated in dedicated module. In contrast |
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.
| parts of an application and can't be isolated in dedicated module. In contrast | |
| parts of an application and can't be isolated in a dedicated module. In contrast |
| Properly handling errors is a complex matter. It is [cross-cutting | ||
| concern](https://en.wikipedia.org/wiki/Cross-cutting_concern), it touches all | ||
| parts of an application and can't be isolated in dedicated module. In contrast | ||
| to several other main stream languages, OCaml provides several mechanisms to |
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 several other main stream languages, OCaml provides several mechanisms to | |
| to several other mainstream languages, OCaml provides several mechanisms to |
| concern](https://en.wikipedia.org/wiki/Cross-cutting_concern), it touches all | ||
| parts of an application and can't be isolated in dedicated module. In contrast | ||
| to several other main stream languages, OCaml provides several mechanisms to | ||
| handled exceptional circumstances, all with good runtime performances and 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.
| handled exceptional circumstances, all with good runtime performances and code | |
| handle exceptional circumstances, all with good runtime performance and code |
| to several other main stream languages, OCaml provides several mechanisms to | ||
| handled exceptional circumstances, all with good runtime performances and code | ||
| understandability. Using them properly requires some initial learning and | ||
| partice. Later, it always require some thinking, which is good since proper |
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.
| partice. Later, it always require some thinking, which is good since proper | |
| practice. Later, it always requires some thinking, which is good since proper |
| understandability. Using them properly requires some initial learning and | ||
| partice. Later, it always require some thinking, which is good since proper | ||
| management of errors shouldn't ever be overlooked. No error handling is better | ||
| than the others, and is should be matter of adequacy to the context rather some |
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.
| than the others, and is should be matter of adequacy to the context rather some | |
| than the others, and is should be matter of adequacy to the context rather than |
christinerose
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.
Mostly grammar/syntax suggestions. A few verb tense and typo corrections.
I didn't go over the code itself yet (to see if there are any gaps for a relative newbie), but I will shortly if you'd like.
| ``` | ||
|
|
||
| And you will get a stacktrace. Alternatively, you can call, from within the program, | ||
| And you will get a stack trace. Alternatively, you can call, from within the program, |
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.
| And you will get a stack trace. Alternatively, you can call, from within the program, | |
| This will produce a stack trace. Alternatively, you can call, from within the program, |
(if my suggestion is correct!)
| Here, it would be wrong to use `failwith` since it requires the compiler to be | ||
| bugged or the system to be corrupted for second code path to be executed. | ||
| Breakage of the language semantics qualifies as extraordinary circumstances, it | ||
| is catastrophic. |
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.
| Here, it would be wrong to use `failwith` since it requires the compiler to be | |
| bugged or the system to be corrupted for second code path to be executed. | |
| Breakage of the language semantics qualifies as extraordinary circumstances, it | |
| is catastrophic. | |
| Here, it wouldn't be beneficial to use `failwith` since it requires the compiler to be | |
| bugged or the system to be corrupted for second code path to be executed. | |
| Breakage of the language semantics qualifies as extraordinary circumstances; it | |
| is catastrophic. |
| # Concluding Remarks | ||
|
|
||
| Properly handling errors is a complex matter. It is [cross-cutting | ||
| concern](https://en.wikipedia.org/wiki/Cross-cutting_concern), it touches all |
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.
| concern](https://en.wikipedia.org/wiki/Cross-cutting_concern), it touches all | |
| concern](https://en.wikipedia.org/wiki/Cross-cutting_concern); it touches all |
| to several other main stream languages, OCaml provides several mechanisms to | ||
| handled exceptional circumstances, all with good runtime performances and code | ||
| understandability. Using them properly requires some initial learning and | ||
| partice. Later, it always require some thinking, which is good since proper |
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.
| partice. Later, it always require some thinking, which is good since proper | |
| pratice. Later, it always require some thinking, which is good since proper |
| handled exceptional circumstances, all with good runtime performances and code | ||
| understandability. Using them properly requires some initial learning and | ||
| partice. Later, it always require some thinking, which is good since proper | ||
| management of errors shouldn't ever be overlooked. No error handling is better |
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.
| management of errors shouldn't ever be overlooked. No error handling is better | |
| error management shouldn't ever be overlooked. No error handling is better |
| understandability. Using them properly requires some initial learning and | ||
| partice. Later, it always require some thinking, which is good since proper | ||
| management of errors shouldn't ever be overlooked. No error handling is better | ||
| than the others, and is should be matter of adequacy to the context rather some |
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.
| than the others, and is should be matter of adequacy to the context rather some | |
| than the others, and it should be matter of adequacy to the context rather some |
|
Thanks, @christinerose for your suggestions, that's great. I've committed them all. Yes if you want to test the code, that be nice too. |
| Exceptions belong to the type `exn` (an extensible sum type): | ||
| The biggest issue with exceptions is that they do not appear in types. One has | ||
| to read the documentation to see that, indeed, `List.find` or `String.sub` are | ||
| not total functions, and that they might fail by raising an exception. |
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.
Total functions? Could benefit from unwrapping.
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.
Can you explain a little bit? I don't get what you mean
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.
Sure thing!
Since the document is intended for newcomers, those might include people unfamiliar with functional programming terminology. I would propose either a quick sidenote or a link to a definition, preferably on our site instead of on some other language's docs.
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 document on functional programming needs to be updated. I was planning to add that kind of text there. I'll link to it
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.
Sounds like the right place.
|
Brilliant! |
|
Thanks, @rikusilvola. BTW, I considered adding some text on how to deal with permanent vs transient failures, à la SRE. Might make sense. I have vague memories of a Google documentation where code patterns where put in front of various kinds of failures, but I couldn't find it. Does not seem to be in the SRE book. Does it ring a bell to you? |
christinerose
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.
Approving in the case all my questions/comments are irrelevant! 😄
Several expressions don't run, and it's not clear to me if they're supposed to run/output, be combined with something else to try it, or if it's just examples/information. I've left notes on several of those, but I stopped about 2/3 through to reduce the noise, as indicated in the last question about it.
A few expressions were missing the ;; to get the output, and I found one or two output errors/discrepancies.
I hope all this is helpful! Let me know about the code questions, and I can review it again. 🙂
| - : int option = None | ||
| # String.sub "Hello world!" 3 (-2);; | ||
| Exception: Invalid_argument "String.sub / Bytes.sub". | ||
| # let rec loop x = x :: loop x |
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.
| # let rec loop x = x :: loop x | |
| # let rec loop x = x :: loop x;; |
| And you will get a stack trace. Alternatively, you can call, from within the | ||
| program, | ||
|
|
||
| ```ocaml |
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.
Is the code li 160 - 191 examples of code the user will write themselves, or is it something the reader should be able to test when going through this doc?
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.
Again, that's for reading only
| | Some host_len -> Some (String.sub fqdn 0 host_len) | ||
| | None -> if fqdn <> "" then Some fqdn else None | ||
| end | ||
| | None -> None |
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.
If I put the ;; at this end of this, I get this output val host_opt : string -> string option = <fun>
Since this keeps-on being a pain, I believe we have a more profound issue with code samples. I've filed this issue on the matter: #961 Let me know if it captures the difficulties you have. Starting from that we can consider, workarounds, to ease your work now and longer-termed solutions. |
OMG! Thank you so much for making this PR. The comments that I already see are very validating that this could be done more clearly. I look forward to exploring the options with the team. |
Hi @cuihtlauac ! No, I don't remember that from Google, but Microsoft has some good guidelines for transient fault handling as well as patterns for error handling for resilient applications. Maybe that's what you remember? |
It must be it. Do you have the link or do you have tips on how I could find it myself? |
https://learn.microsoft.com/en-us/azure/architecture/best-practices/transient-faults |
|
Thanks @rikusilvola. It's not what I had in mind. But it's great stuff! I'll see what I can do from there. |
|
Excellent work, I find it very well explained and the code examples are clearer and more detailed. |
|
(I don't know if this is the right place to give general comments, I can also reply on the discuss post. Also sorry if the points below have been taken into account already).
|
|
The introduction tells the reader not to use special values for error handling but does not say why. I made a new sample intro that gives a small hint as to the problems with special values.
|
|
Thanks, @Tchou, for the nice words, this helps. To me, both here and discuss are fine for general comments.
I believe the initial goal of the document was to address all exceptional circumstances; that's why I felt those topics were needed. However, I share your concerns. Maybe I should add some text saying this is mentioned for completeness but left unexplained. As a beginner, I preferred that over “being left in the dark”.
Agreed, will do
Agreed, will do.
There's an open issue on that matter: #961 Since this applies to several tutorials, we'll probably won't fix this in this document, in order to preserve formatting consistency. Hopefully, it will help solve the issue at once for all the documents. |
Grammar fixes Co-authored-by: Christine Rose <[email protected]>
Co-authored-by: Miod Vallat <[email protected]>
Co-authored-by: Christine Rose <[email protected]>
sabine
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.
Just one remark.
In general, I think it's good to merge and improve incrementally from there on.
Discuss thread: https://discuss.ocaml.org/t/updating-the-error-handling-tutorial/12022