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

refactor: more error info, span merge #4470

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

m-span
Copy link
Contributor

@m-span m-span commented May 13, 2024

  • merges ast into parser moves error from span to parser
  • merges span, error objects into single, custom objects
  • handles error struct conversion better so errors are more informative
  • fixes existing bad error message for interpolated strings
  • adds new bad error message for interpolated strings

@m-span m-span changed the title [WIP] Error Refactor, CLI Logging Error Refactor, CLI Logging May 13, 2024
Copy link
Member

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

Nice, great to see you've made progress!

It would be easier to review if we can clean up some of the diffs — looks like we have some merge conflicts and some formatting-only changes. (I don't think we need to be sticklers about splitting things into commits within a PR.)

It also would be easier to review if we can split things into smaller PRs; for example there's a nice -v option that we could merge immediately. Review difficulty seems to increase super-linearly with size, so breaking things up is easier. But it's also not fun to do ex-post, so it may not be worthwhile at this stage.

Does the prqlc-ast / prqlc-parser crate merging give us anything at the moment? I'm hesitant to go through another refactor without either a clear reduction in code complexity or a win on language functionality...

Nice work on the error message improvements — that part sounds great.

prqlc/prqlc/src/cli/mod.rs Outdated Show resolved Hide resolved
@m-span m-span changed the title Error Refactor, CLI Logging Error Refactor May 14, 2024
@m-span m-span changed the title Error Refactor refactor: more error info, span merge May 14, 2024
@m-span
Copy link
Contributor Author

m-span commented May 16, 2024

Dupilicate code and concepts in span and error were merged. I moved ast files back to prqlc-ast, except for error, which had to stay in the parser to avoid circular deps.

@m-span m-span marked this pull request as ready for review May 16, 2024 05:07
Copy link
Member

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

Superb, thanks!

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.

None yet

2 participants