Update of the manual to document error reporting #318
Conversation
panglesd
requested review from
ceastlund,
NathanReb and
pitag-ha
as code owners
|
I moved mention of |
pitag-ha
added
the
no changelog
There was a problem hiding this comment.
Hey @panglesd, thanks for the section on error reporting, which is really needed! I haven't read it till the end yet, but I'm already leaving you some comments.
I think the beginning is very good! Then there comes a paragraph about whole-file transformations (see my comment below), then a paragraph about which I'm not sure if it's a summary or not (see my comment below), and then there come some sections that really go a lot into detail.
About the sections that go a lot into detail: what do you want the user to get out of them? And who's the target? (I myself can see different purposes for them, but I'm curious what the purpose was you had in mind when writing them)
Also, maybe the problem here is that the two of us just have very different writing/reading styles, so if you're very confident with how it is now, feel free to just merge it as is!
(Btw, I've only corrected the English of some paragraphs that stuck out to me. If you want, you could pass this through some grammar corrector (not necessary though).)
|
Unfortunately, I think you have been reviewing an "rst comments" that is thus not included in the rendering of the manual... Now that I have more confidence that #325 will be merged without deep modification, I think the best is that I rebase this PR on top of #325, migrate this section to odoc, implement your comments whenever they are not targeted to a "commented" part, and you resume your review from there? In particular, I am not sure whether the comments about the structure still apply, without commented block! |
panglesd
force-pushed
the
manual-panglesd
branch
from
fdad824
to
304f165
Compare
There was a problem hiding this comment.
Perfect, thanks a lot for the changes! I think it makes far clearer now how to report errors in a good way. I think it now has a very good balance between the important more or less high-level descriptions and just one low-level example.
My comments below are just suggestions, nothing important. For me, we can merge!
| ]} | ||
|
|
||
|
|
||
| When the record definition contains several fields with types other than [int], [float] or [string], several error nodes are added in the AST. Moreover, the location of the error nodes corresponds to the definition of the record fields. This allows tools such as Merlin to report all errors at once, at the right location, resulting in a better workflow than having to recompile everytime one error is corrected to see the next one. |
There was a problem hiding this comment.
That's interesting! That sounds like the only feature merlin loses when raising a located error instead of embedding the error is reporting multiple ppx errors. In fact, merlin looses all its features inside the context of a ppx, doesn't it? One of those features it looses of course is the reporting of more ppx errors, but others are jump-to-definition or show-definition-on-hover inside payloads etc, right? (this is just a side-note; no need to change anything)
There was a problem hiding this comment.
I would say that merlin does not lose those features as, with ppxlib 0.25, the last valid AST is kept when catching an exception, but I might not have understand exactly what you mean by "inside payloads"!
There was a problem hiding this comment.
With "inside payload" I meant something like
let%my_ppx = <super long payload composed of lots of different value bindings etc
that, iiuc, don't have merlin features anymore if my_ppx raises>
But we can leave this sentence as is!
|
I added a section explaining how to update a codebase to use the embedding error mechanism. |
Nice, thanks! Hopefully, that will convince even more people to improve their error reporting now. It still all looks good to me. Let's merge this! |
|
Oh wait, one thing I've just noticed: we could give this a changelog entry to give it more visibility. What do you think? |
panglesd
force-pushed
the
manual-panglesd
branch
from
db7395e
to
ea3a67f
Compare
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]> Co-authored-by: Sonja Heinze <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
panglesd
force-pushed
the
manual-panglesd
branch
from
ea3a67f
to
ca8a207
Compare
CHANGES: - Update expansion context to leave out value name when multiple are defined at once. (ocaml-ppx/ppxlib#351, @ceastlund) - Add support for OCaml 5.0 (ocaml-ppx/ppxlib#348, @pitag-ha) - Add `Code_path.enclosing_value` (ocaml-ppx/ppxlib#349, @ceastlund) - Add `Code_path.enclosing_module` (ocaml-ppx/ppxlib#346, @ceastlund) - Expand code generated by `~enclose_intf` and `~enclose_impl` (ocaml-ppx/ppxlib#345, @ceastlund) - Add type annotations to code generated by metaquot (ocaml-ppx/ppxlib#344, @ceastlund) - Fix typo in description field of dune-project (ocaml-ppx/ppxlib#343, @ceastlund) - Fix Ast_pattern.many (ocaml-ppx/ppxlib#333, @nojb) - Fix quoter and optimize identifier quoting (ocaml-ppx/ppxlib#327, @sim642) - Driver, when run with `--check`: Allow `toplevel_printer` attributes (ocaml-ppx/ppxlib#340, @pitag-ha) - Documentation: Add a section on reporting errors by embedding extension nodes in the AST (ocaml-ppx/ppxlib#318, @panglesd) - Driver: In the case of ppxlib internal errors, embed those errors instead of raising to return a meaningful AST (ocaml-ppx/ppxlib#329, @panglesd) - API: For each function that could raise a located error, add a function that return a `result` instead (ocaml-ppx/ppxlib#329, @panglesd)
This PR is part of the #314 tasks.
It adds a section about error reporting in the
ppx for plugin authorssection.Note that it assumes that #316 has been merged, as it mentions the new API.