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

Embed error into AST #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Embed error into AST #46

wants to merge 1 commit into from

Conversation

Annosha
Copy link

@Annosha Annosha commented Mar 24, 2023

This PR aims to address this issue #45

Modified functions: rest_must_be_empty and check_expectation_body
check_expectation_body function calls the modified rest_must_be_empty function, and the error location and message are passed as arguments to the Error variant of the result type.

@panglesd
Copy link

However, when I run dune build I'm getting multiple errors. (Not sure if this library is supposed to run as a dune build)

I think that you should base your changes of the v0.15.1 tag. Several libraries are supposed to be developed in parallel, and the error you are seeing are due to that.

Your code also contains errors, though! But at least, dune build will guide you better, once you rebase your commit of v.0.15.1.

Some questions/remarks:

  • Why did you decide to move the rest_must_be_empty function?
  • What does the check_expectation_body function?
  • Using Ok and Error is a good idea, but in the Ok case, it seems no value is returned...
  • You'll need to fix some syntax!

@Annosha
Copy link
Author

Annosha commented Mar 24, 2023

@panglesd
I moved the rest_must_be_empty function because it is only used in thePrettycase of the body match, and I wanted to avoid defining functions that are not used elsewhere in the code.

The check_expectation_body function checks the body of an expectation to ensure that it conforms to the multi-line expectation format. It returns a result type indicating whether the body is valid (Ok) or contains an error (Error). The error message and location are embedded in the Error variant of the result type.

@Annosha
Copy link
Author

Annosha commented Mar 28, 2023

@panglesd I'm having confusion about how to embed an error into AST. Is there a complete example of how to go about solving these issues? I struggle to identify the exact lines of code to modify and understand what effect they have.

@panglesd
Copy link

Are your issues more technical (how to implement it) or more general (understanding what "embedding errors in the AST" means)?

There is one example that can help to understand the concept: the ppxlib's examples used to raise errors, and they now embed them in the AST.
The raising version of the examples can be found here.
The embedding version of the examples can be found here.
You should use these examples, not to try to copy them in the new context of ppx_expect, but to understand well how to embed errors into an AST.

Also, it seems that you are not completely comfortable with the syntax (your PR is not yet syntactically valid!). OCaml gives quite unhelpful messages in case of syntax error, but quite helpful (although sometimes hard to read) messages on syntactically correct file. So if your issue is more technical, fixing your syntax errors might yield better error messages.

@github-iron github-iron added the forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system. label Mar 31, 2023
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.

None yet

3 participants