-
Notifications
You must be signed in to change notification settings - Fork 221
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!: prqlc-parser major reorg changes, remove prqlc-ast #4634
Conversation
still a wip, need to make sure all references are updated correctly, but feel free to take a look and comment at how |
Looking great at first glance! (Unfortunately it looks like it may not be based off the latest |
Super minor but possibly standards are for |
There's more work to be done, but this is a good first step. We might need to talk about how we want to be re-exporting our imports. |
I tried to do the least amount of changes that accomplishes the goal, but its still a lot. Requesting many eyes on this. Also, some test are suddenly failing - trying to figure it out, but if anyone has any solutions, feel free to suggest or push. |
Yes perfect. I have a few small things to merge, and possibly a fairly big thing in #4639, so great if we can do these in smallish PRs with smaller conflicts. A couple test failures indeed look surprising. Sometimes I realize I'm importing the wrong (a couple are easy clippy ones, will probably auto-fix with |
there seems to be issues with the |
No longer needed; we're not going to have multiple copies of AST items any longer...
Funny you asked, I actually just pushed a change removing them. (They existed as a compromise to having multiple copies of similar AST structs around...) |
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.
Same here — IIUC it would be strictly better to have only one of these — no strong view on which
I'm sure we'll find some more refinements but looking really good for the moment — will hit the big button. I made some small-ish refinements, feel free to object to anything if I went against your intention... |
(could definitely do with a changelog entry, doesn't need to be part of this PR tho) |
Looking good from this side. Cleanups were great. Just a small shame I coudln't get the refactor on |
I'll add it. |
Let's add (your approach is reasonable but it may not generalize well to multiple breaking changes in a release without another mechanism...) |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Thank you @m-span ! |
From #4603 , we have the following new terminology:
Goals:
prqlc-ast
toprqlc-parser
.prqlc-ast
is to be deprecated