Update documentation with new behaviour wrt exceptions #457
Conversation
panglesd
added
the
no changelog
| [[%ocaml.error "error_message"]] that can be embedded into a valid AST and are interpreted later | ||
| as errors, e.g., by the compiler or Merlin. As all extension nodes, they can be | ||
| put {{:https://ocaml.org/manual/extensionnodes.html}at many places in the AST} | ||
| to replace structure items, expressions, or patterns, for example. | ||
|
|
||
| So whenever you're in doubt whether to throw an exception or if to embed the error as |
There was a problem hiding this comment.
Hello @panglesd , I was thinking we update this if there is no longer a preferred way of reporting errors, except the new behaviour does not still make raising as good as embedding.
There was a problem hiding this comment.
Yes, even though it's not so important anymore to migrate code from exceptions to embedding, since the behavior is acceptable, it is still better if new PPX adopts the embedding way of reporting errors.
Also, this paragraph speaks about where to embed the errors (as deep as possible) so it makes sense to keep it.
doc/good-practices.mld
Outdated
| generated code. | ||
| - By raising a sepcific exception, letting the ppxlib driver handle the error. | ||
|
|
||
| Let us already emphasize that, while exceptions can be practical to quickly fail |
There was a problem hiding this comment.
I am thinking we could maybe remove already in this sentence.
panglesd
force-pushed
the
doc-for-errors
branch
from
799b3e5 to
a24882b
Compare
There was a problem hiding this comment.
Perfect! Thank you, @panglesd and @Burnleydev1, both for the update of the docs and the implementation of this! Btw, do you want me to have a look at the PRs of the implementation, @panglesd?
doc/good-practices.mld
Outdated
| {{!Ppxlib.Location.raise_errorf}[Location.raise_errorf]}. But keep in mind that | ||
| in many case, there is a better AST to continue from than the one that was | ||
| initially passed to the rewriter. |
There was a problem hiding this comment.
What do you mean with the "keep in mind" side-note here?
There was a problem hiding this comment.
I wanted to say that raise_errorf should only be used when there is no advantages to embed the error in the AST. But that's not even true: a PPX can raise a located error, and catch it at the point where it wants to embed the error, instead of using result type.
I'll remove the side-note.
If you have time, yes! |
panglesd
force-pushed
the
doc-for-errors
branch
from
74cdde5 to
e94e3d2
Compare
There was a problem hiding this comment.
It looks great! Very clear!
My only concern is that it goes quite a bit into detail on how we handle exceptions while we recommend not using them.
Do you have another section in mind where we could document this behaviour and keep this section about what we recommend?
If not I think I would at least swap the order of the sub sections and put the recommendation first and only then explain how we deal with exceptions now!
|
Thanks for the review!
That's a good point! For a smaller change, putting this explanation after the better practice of embedding (or even after the documented example) seems a good improvement already. |
|
Sure, happy to take care of it! |
NathanReb
force-pushed
the
doc-for-errors
branch
from
e94e3d2 to
1033e37
Compare
Signed-off-by: Paul-Elliot <[email protected]> Signed-off-by: Nathan Rebours <[email protected]>
Co-authored-by: Sonja Heinze <[email protected]> Signed-off-by: Paul-Elliot <[email protected]> Signed-off-by: Nathan Rebours <[email protected]>
Signed-off-by: Nathan Rebours <[email protected]>
NathanReb
force-pushed
the
doc-for-errors
branch
from
1033e37 to
6dfa0b8
Compare
|
Just moved the exception handling section to |
|
@panglesd if this looks good to you, I'll go ahead and merge! |
Signed-off-by: Paul-Elliot <[email protected]>
|
I can't approve my own PR but I do approve! I think it is good to merge. I went ahead and added some links (the "exception handling" explanation used the concept of "context-free transformations" that are unavailable in this earlier part of the doc ; and the way exceptions are handled was a bit too disconnected from the good practices part). Feel free to revert if you don't like them! |
This is the PR that updates the documentation taking into account the changes made in #447 and #453.
Pinging @Burnleydev1 if you want to review!
This PR makes it "more acceptable" to raise for PPXs, since it is supported and does not harm the other ppx execution. So most of the changes are in the "raising" section of the manual.