-
Notifications
You must be signed in to change notification settings - Fork 205
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
Dupilicate code and concepts in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superb, thanks!
merges ast into parsermoves error from span to parser