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

ppx_fields_conv: Embed errors in the AST instead of raising #11

Closed
wants to merge 2 commits into from
Closed

ppx_fields_conv: Embed errors in the AST instead of raising #11

wants to merge 2 commits into from

Conversation

Burnleydev1
Copy link

Coses #10

Hello @panglesd here is a draft pr for the issue, It still has errors which I was talking about in the issue.

@panglesd
Copy link

In the metaquot issue, the error happened at a point where it could be directly turned into an AST node: the function was outputing an expression, so you could directly embed the error in an expression and return this.

Here, the errors are checked in a function, which:

  • does nothing if everything goes well (returns unit)
  • raises otherwise.

So the return type of the function has to be changed! For instance, it could return a result type, as explained for instance in realworldofocaml. In this case, the function could simply return the list of errors. At the call site of the function, if it returns an empty list, continue as usual, otherwise embed the errors in the relevant AST node!

@github-iron github-iron added the forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system. label Mar 21, 2023
@Burnleydev1 Burnleydev1 changed the title ppx_fields_conv Embed errors in the AST instead of raising ppx_fields_conv: Embed errors in the AST instead of raising Mar 21, 2023
@Burnleydev1
Copy link
Author

Thank you @panglesd I get on this right away.

@Burnleydev1 Burnleydev1 deleted the error_embed branch March 27, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants