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

Add error_extensionf in the API to ease the process of embedding errors #316

Merged
merged 8 commits into from
Feb 3, 2022

Conversation

panglesd
Copy link
Collaborator

This is part of the #314 tasks.

The examples are updated to embed errors using the new function, instead of raising located errors.

@panglesd panglesd changed the title And error_extensionf in the API to ease the process of embedding errors Add error_extensionf in the API to ease the process of embedding errors Jan 12, 2022
Copy link
Member

@pitag-ha pitag-ha left a comment

Choose a reason for hiding this comment

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

Perfect, LGTM. Thanks, @panglesd! Could you add a changelog entry?

@pitag-ha
Copy link
Member

Oh, and one more thing: we should also edit the documentation of Location.raise_errorf to point out that that function should be avoided and error_extensionf should be used instead. Also, the sentence "The exception is caught by the driver and handled appropriately" is very confusing, if not wrong. What do you think? Do you want to update that here or rather in a separate PR?

@panglesd
Copy link
Collaborator Author

Sure! I added a changelog entry and updated the doc.

When this PR and the #318 PR are merged, there should be a link from the API of those functions to the relevant part of the manual. The maintenance of this link would be eased if the manual was written using odoc 😉

Copy link
Member

@pitag-ha pitag-ha left a comment

Choose a reason for hiding this comment

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

Perfect, thanks! Let's move the changelog entry to the unreleased section and merge this.

CHANGES.md Outdated
@@ -19,6 +19,8 @@
consider a type declaration recursive if the type appeared inside an attribute
payload (#299, @NathanReb)

- Added `error_extensionf` function to the `Locate` module (#316, @panglesd)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Added `error_extensionf` function to the `Locate` module (#316, @panglesd)
- Added `error_extensionf` function to the `Location` module (#316, @panglesd)

Also, similarly to here: would you mind rebasing over main and moving this entry up to the unreleaed section (it's currently in the section of the last release).

Signed-off-by: Paul-Elliot <[email protected]>
Copy link
Member

@pitag-ha pitag-ha left a comment

Choose a reason for hiding this comment

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

Perfect, thank you!

@pitag-ha pitag-ha merged commit 132b73f into ocaml-ppx:main Feb 3, 2022
kit-ty-kate pushed a commit to ocaml/opam-repository that referenced this pull request Mar 3, 2022
CHANGES:

- Added `error_extensionf` function to the `Location` module (ocaml-ppx/ppxlib#316, @panglesd)

- Ast patterns: add `drop` and `as` patterns (ocaml-ppx/ppxlib#313 by @Kakadu, review by @pitag-ha)

- Fixed a bug resulting in disscarded rewriters in the presence of
  instrumentations, as well as a wrong order of rewriting (ocaml-ppx/ppxlib#296, @panglesd)

- Driver: Append the last valid AST to the error in case of located exception
  when embedding errors (ocaml-ppx/ppxlib#315, @panglesd)
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.

2 participants