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

ast.Errors #5122

Closed
wants to merge 1 commit into from
Closed

ast.Errors #5122

wants to merge 1 commit into from

Conversation

mattnibs
Copy link
Collaborator

This commit introduces ast.Errors- an error type that should be used whenever a generated error refers to a place in the query source. This commit switches the parser package to use ast.Error but a follow up commit will use ast.Errors for the semantic package as well.

@mattnibs mattnibs requested a review from a team May 13, 2024 23:50
@mattnibs mattnibs force-pushed the ast-error branch 3 times, most recently from 261d933 to 55d357c Compare May 13, 2024 23:56
This commit introduces ast.Errors- an error type that should be used
whenever a generated error refers to a place in the query source. This
commit switches the parser package to use ast.Error but a follow up
commit will use ast.Errors for the semantic package as well.
@@ -34,34 +34,34 @@ outputs:
- name: stderr
data: |
=1=
error parsing Zed at column 11:
error parsing Zed (line 1, column 11):
Copy link
Member

@nwt nwt May 18, 2024

Choose a reason for hiding this comment

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

This looks like a purely cosmetic change. Can we save it for a separate PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really since with semantic errors you have different error messages that may not syntactically work in the "append this to a sentence" way that errors are currently formatted.

Comment on lines +25 to +26
Query(ctx context.Context, head *lakeparse.Commitish, src string) (zio.ReadCloser, error)
QueryWithControl(ctx context.Context, head *lakeparse.Commitish, src string) (zbuf.ProgressReadCloser, error)
Copy link
Member

Choose a reason for hiding this comment

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

Do these APIs need to change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it should be this way since if you let lake/api/api do the ConcatSources for you won't be able to map error locations to your source set.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that the mapping would continue to happen inside these methods in the same way it effectively does now. Shifting that burden to the caller seems like it makes for a more complicated interface since the caller now has to deal with parser.ConcatSource and clierrors.Format.

Does shifting that burden have some benefit I'm not seeing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I eventually want to rewrite the error text to use asni escape sequences for pretty console output- where should the package that formats the error messages go?

@mattnibs
Copy link
Collaborator Author

Closing in favor of #5128

@mattnibs mattnibs closed this May 31, 2024
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