Skip to content

refactor!: prqlc-parser major reorg changes, remove prqlc-ast - #4634

Merged
max-sixty merged 16 commits into
PRQL:mainfrom
m-span:renaming
Jun 20, 2024
Merged

refactor!: prqlc-parser major reorg changes, remove prqlc-ast#4634
max-sixty merged 16 commits into
PRQL:mainfrom
m-span:renaming

Conversation

@m-span

@m-spanm-span commented Jun 18, 2024•
edited
Loading

Copy link
Copy Markdown
Collaborator

From #4603 , we have the following new terminology:

stagesub-stageAST
parselexerstring -> LR — Lexer Representation
parseparsertokens -> PR — Parser Representation
semanticast_expandPR -> PL — Pipelined Language
semanticresolverPL
semanticflattenPL
semanticloweringPL -> RQ — Resolved Query
sqlpreprocessRQ
sqlsrq-compilerRQ -> PQ — Partitioned Query
sqlpostprocessPQ
sqlsql-compilerPQ -> sqlparser::ast
sqlcodegensqlparser::ast -> string

Goals:

  • Move all files from prqlc-ast to prqlc-parser. prqlc-ast is to be deprecated
  • Within parser stage, rename objects, modules and methods to meet new spec. Organize lexer and parser stage into separate modules.

@m-span

m-span commented Jun 18, 2024

Copy link
Copy Markdown
CollaboratorAuthor

still a wip, need to make sure all references are updated correctly, but feel free to take a look and comment at how prqlc-parser is now organized

@max-sixty

max-sixty commented Jun 18, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Looking great at first glance!

(Unfortunately it looks like it may not be based off the latest main and so have some conflicts; worth pulling the latest main... really hope that's not a painful merge. To the extent it's caused by #4522, there are instructions there for how to reproduce that; so running the same commands on this branch should produce a very similar, easily-mergable result)

@max-sixty

max-sixty commented Jun 18, 2024

Copy link
Copy Markdown
Member

Super minor but possibly standards are for pr to be lowercase; no view from me beyond following the rust naming standards

@m-span
m-span marked this pull request as ready for review June 20, 2024 00:00
@m-span

m-span commented Jun 20, 2024

Copy link
Copy Markdown
CollaboratorAuthor

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.
These changes are absolutely Major version number update-API-breaking changes, since we are re-naming our objects.

@m-span

m-span commented Jun 20, 2024

Copy link
Copy Markdown
CollaboratorAuthor

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.

max-sixty
max-sixty reviewed Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lib.rs Outdated
@max-sixty

max-sixty commented Jun 20, 2024

Copy link
Copy Markdown
Member

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 Expr etc; so I guess that's possible here?!

(a couple are easy clippy ones, will probably auto-fix with clippy --fix)

@m-span

m-span commented Jun 20, 2024

Copy link
Copy Markdown
CollaboratorAuthor

there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...

@max-sixty
Remove ast_code_matches file
f3bb696
No longer needed; we're not going to have multiple copies of AST items any longer...
@max-sixty

max-sixty commented Jun 20, 2024

Copy link
Copy Markdown
Member

there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...

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...)

max-sixty
max-sixty reviewed Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty reviewed Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty reviewed Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/error/mod.rs
max-sixty
max-sixty reviewed Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/mod.rs
max-sixty
max-sixty reviewed Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/lr/mod.rs

@max-sixtymax-sixtyJun 20, 2024•
edited
Loading

Copy link
Copy Markdown
Member

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

@max-sixty

max-sixty commented Jun 20, 2024

Copy link
Copy Markdown
Member

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...

@max-sixty
max-sixty enabled auto-merge (squash) June 20, 2024 01:52
@max-sixty

max-sixty commented Jun 20, 2024

Copy link
Copy Markdown
Member

(could definitely do with a changelog entry, doesn't need to be part of this PR tho)

@m-span

m-span commented Jun 20, 2024

Copy link
Copy Markdown
CollaboratorAuthor

Looking good from this side. Cleanups were great. Just a small shame I coudln't get the refactor on prqlc_parser::parse_source() working that I had to undo it via this commit.
It did some nice things like hide stuff that didn't need to be exposed

@m-span

m-span commented Jun 20, 2024

Copy link
Copy Markdown
CollaboratorAuthor

(could definitely do with a changelog entry, doesn't need to be part of this PR tho)

I'll add it.
Also, this is a breaking api change...should we increment the working version number to a major increment?

@m-span
m-span disabled auto-merge June 20, 2024 02:13
@max-sixty

max-sixty commented Jun 20, 2024

Copy link
Copy Markdown
Member

Also, this is a breaking api change...should we increment the working version number to a major increment?

Let's add ! in the PR description and then we'll do it at release time

(your approach is reasonable but it may not generalize well to multiple breaking changes in a release without another mechanism...)

@max-sixtymax-sixty changed the title refactor: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, deprecate prqlc-astJun 20, 2024
max-sixty
max-sixty reviewed Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md Outdated
max-sixty
max-sixty reviewed Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md Outdated
@max-sixty
max-sixty enabled auto-merge (squash) June 20, 2024 02:46
@m-spanm-span mentioned this pull request Jun 20, 2024
Closed
@max-sixtymax-sixty changed the title refactor!: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, remove prqlc-astJun 20, 2024
@max-sixty
max-sixty merged commit 6357126 into PRQL:mainJun 20, 2024
@max-sixty

max-sixty commented Jun 20, 2024

Copy link
Copy Markdown
Member

Thank you @m-span !

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@max-sixtymax-sixtymax-sixty left review comments
@aljazerzenaljazerzenAwaiting requested review from aljazerzen

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@m-span@max-sixty
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
refactor!: prqlc-parser major reorg changes, remove prqlc-ast by m-span · Pull Request #4634 · PRQL/prql · GitHub
Skip to content
refactor!: prqlc-parser major reorg changes, remove prqlc-ast - #4634
Merged
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Jun 20, 2024
Merged
refactor!: prqlc-parser major reorg changes, remove prqlc-ast#4634
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Conversation
@m-span
@m-spanm-span
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
From #4603 , we have the following new terminology:
stagesub-stageAST
parselexerstring -> LR — Lexer Representation
parseparsertokens -> PR — Parser Representation
semanticast_expandPR -> PL — Pipelined Language
semanticresolverPL
semanticflattenPL
semanticloweringPL -> RQ — Resolved Query
sqlpreprocessRQ
sqlsrq-compilerRQ -> PQ — Partitioned Query
sqlpostprocessPQ
sqlsql-compilerPQ -> sqlparser::ast
sqlcodegensqlparser::ast -> string
Goals:
  •  Move all files from prqlc-ast to prqlc-parser. prqlc-ast is to be deprecated
  •  Within parser stage, rename objects, modules and methods to meet new spec. Organize lexer and parser stage into separate modules.
@m-span
m-span
commented
Jun 18, 2024

Copy link


Copy Markdown

CollaboratorAuthor
still a wip, need to make sure all references are updated correctly, but feel free to take a look and comment at how prqlc-parser is now organized
@max-sixty
max-sixty
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Looking great at first glance!
(Unfortunately it looks like it may not be based off the latest main and so have some conflicts; worth pulling the latest main... really hope that's not a painful merge. To the extent it's caused by #4522, there are instructions there for how to reproduce that; so running the same commands on this branch should produce a very similar, easily-mergable result)
@max-sixty
max-sixty
commented
Jun 18, 2024

Copy link


Copy Markdown

Member
Super minor but possibly standards are for pr to be lowercase; no view from me beyond following the rust naming standards
@m-span
m-span
marked this pull request as ready for review
June 20, 2024 00:00
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.

These changes are absolutely Major version number update-API-breaking changes, since we are re-naming our objects.
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lib.rs
Outdated

@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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 Expr etc; so I guess that's possible here?!
(a couple are easy clippy ones, will probably auto-fix with clippy --fix)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
@max-sixty
Remove ast_code_matches file
f3bb696
No longer needed; we're not going to have multiple copies of AST items any longer...
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
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...)
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/error/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/lr/mod.rs
@max-sixtymax-sixtyJun 20, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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...
@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 01:52
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
Looking good from this side. Cleanups were great. Just a small shame I coudln't get the refactor on prqlc_parser::parse_source() working that I had to undo it via this commit.

It did some nice things like hide stuff that didn't need to be exposed
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
I'll add it.

Also, this is a breaking api change...should we increment the working version number to a major increment?
@m-span
m-span
disabled auto-merge
June 20, 2024 02:13
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Also, this is a breaking api change...should we increment the working version number to a major increment?
Let's add ! in the PR description and then we'll do it at release time
(your approach is reasonable but it may not generalize well to multiple breaking changes in a release without another mechanism...)
@max-sixtymax-sixty
changed the title
refactor: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, deprecate prqlc-astJun 20, 2024
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 02:46
@m-spanm-span
mentioned this pull request
Jun 20, 2024

Closed

@max-sixtymax-sixty
changed the title
refactor!: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, remove prqlc-astJun 20, 2024
@max-sixty
max-sixty
merged commit 6357126
into
PRQL:mainJun 20, 2024
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Thank you @m-span !
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@max-sixtymax-sixtymax-sixty left review comments
@aljazerzenaljazerzenAwaiting requested review from aljazerzen

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@m-span@max-sixty

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
refactor!: prqlc-parser major reorg changes, remove prqlc-ast by m-span · Pull Request #4634 · PRQL/prql · GitHub
Skip to content
refactor!: prqlc-parser major reorg changes, remove prqlc-ast - #4634
Merged
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Jun 20, 2024
Merged
refactor!: prqlc-parser major reorg changes, remove prqlc-ast#4634
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Conversation
@m-span
@m-spanm-span
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
From #4603 , we have the following new terminology:
stagesub-stageAST
parselexerstring -> LR — Lexer Representation
parseparsertokens -> PR — Parser Representation
semanticast_expandPR -> PL — Pipelined Language
semanticresolverPL
semanticflattenPL
semanticloweringPL -> RQ — Resolved Query
sqlpreprocessRQ
sqlsrq-compilerRQ -> PQ — Partitioned Query
sqlpostprocessPQ
sqlsql-compilerPQ -> sqlparser::ast
sqlcodegensqlparser::ast -> string
Goals:
  •  Move all files from prqlc-ast to prqlc-parser. prqlc-ast is to be deprecated
  •  Within parser stage, rename objects, modules and methods to meet new spec. Organize lexer and parser stage into separate modules.
@m-span
m-span
commented
Jun 18, 2024

Copy link


Copy Markdown

CollaboratorAuthor
still a wip, need to make sure all references are updated correctly, but feel free to take a look and comment at how prqlc-parser is now organized
@max-sixty
max-sixty
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Looking great at first glance!
(Unfortunately it looks like it may not be based off the latest main and so have some conflicts; worth pulling the latest main... really hope that's not a painful merge. To the extent it's caused by #4522, there are instructions there for how to reproduce that; so running the same commands on this branch should produce a very similar, easily-mergable result)
@max-sixty
max-sixty
commented
Jun 18, 2024

Copy link


Copy Markdown

Member
Super minor but possibly standards are for pr to be lowercase; no view from me beyond following the rust naming standards
@m-span
m-span
marked this pull request as ready for review
June 20, 2024 00:00
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.

These changes are absolutely Major version number update-API-breaking changes, since we are re-naming our objects.
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lib.rs
Outdated

@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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 Expr etc; so I guess that's possible here?!
(a couple are easy clippy ones, will probably auto-fix with clippy --fix)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
@max-sixty
Remove ast_code_matches file
f3bb696
No longer needed; we're not going to have multiple copies of AST items any longer...
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
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...)
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/error/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/lr/mod.rs
@max-sixtymax-sixtyJun 20, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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...
@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 01:52
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
Looking good from this side. Cleanups were great. Just a small shame I coudln't get the refactor on prqlc_parser::parse_source() working that I had to undo it via this commit.

It did some nice things like hide stuff that didn't need to be exposed
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
I'll add it.

Also, this is a breaking api change...should we increment the working version number to a major increment?
@m-span
m-span
disabled auto-merge
June 20, 2024 02:13
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Also, this is a breaking api change...should we increment the working version number to a major increment?
Let's add ! in the PR description and then we'll do it at release time
(your approach is reasonable but it may not generalize well to multiple breaking changes in a release without another mechanism...)
@max-sixtymax-sixty
changed the title
refactor: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, deprecate prqlc-astJun 20, 2024
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 02:46
@m-spanm-span
mentioned this pull request
Jun 20, 2024

Closed

@max-sixtymax-sixty
changed the title
refactor!: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, remove prqlc-astJun 20, 2024
@max-sixty
max-sixty
merged commit 6357126
into
PRQL:mainJun 20, 2024
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Thank you @m-span !
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@max-sixtymax-sixtymax-sixty left review comments
@aljazerzenaljazerzenAwaiting requested review from aljazerzen

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@m-span@max-sixty

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
refactor!: prqlc-parser major reorg changes, remove prqlc-ast by m-span · Pull Request #4634 · PRQL/prql · GitHub
Skip to content
refactor!: prqlc-parser major reorg changes, remove prqlc-ast - #4634
Merged
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Jun 20, 2024
Merged
refactor!: prqlc-parser major reorg changes, remove prqlc-ast#4634
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Conversation
@m-span
@m-spanm-span
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
From #4603 , we have the following new terminology:
stagesub-stageAST
parselexerstring -> LR — Lexer Representation
parseparsertokens -> PR — Parser Representation
semanticast_expandPR -> PL — Pipelined Language
semanticresolverPL
semanticflattenPL
semanticloweringPL -> RQ — Resolved Query
sqlpreprocessRQ
sqlsrq-compilerRQ -> PQ — Partitioned Query
sqlpostprocessPQ
sqlsql-compilerPQ -> sqlparser::ast
sqlcodegensqlparser::ast -> string
Goals:
  •  Move all files from prqlc-ast to prqlc-parser. prqlc-ast is to be deprecated
  •  Within parser stage, rename objects, modules and methods to meet new spec. Organize lexer and parser stage into separate modules.
@m-span
m-span
commented
Jun 18, 2024

Copy link


Copy Markdown

CollaboratorAuthor
still a wip, need to make sure all references are updated correctly, but feel free to take a look and comment at how prqlc-parser is now organized
@max-sixty
max-sixty
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Looking great at first glance!
(Unfortunately it looks like it may not be based off the latest main and so have some conflicts; worth pulling the latest main... really hope that's not a painful merge. To the extent it's caused by #4522, there are instructions there for how to reproduce that; so running the same commands on this branch should produce a very similar, easily-mergable result)
@max-sixty
max-sixty
commented
Jun 18, 2024

Copy link


Copy Markdown

Member
Super minor but possibly standards are for pr to be lowercase; no view from me beyond following the rust naming standards
@m-span
m-span
marked this pull request as ready for review
June 20, 2024 00:00
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.

These changes are absolutely Major version number update-API-breaking changes, since we are re-naming our objects.
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lib.rs
Outdated

@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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 Expr etc; so I guess that's possible here?!
(a couple are easy clippy ones, will probably auto-fix with clippy --fix)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
@max-sixty
Remove ast_code_matches file
f3bb696
No longer needed; we're not going to have multiple copies of AST items any longer...
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
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...)
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/error/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/lr/mod.rs
@max-sixtymax-sixtyJun 20, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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...
@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 01:52
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
Looking good from this side. Cleanups were great. Just a small shame I coudln't get the refactor on prqlc_parser::parse_source() working that I had to undo it via this commit.

It did some nice things like hide stuff that didn't need to be exposed
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
I'll add it.

Also, this is a breaking api change...should we increment the working version number to a major increment?
@m-span
m-span
disabled auto-merge
June 20, 2024 02:13
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Also, this is a breaking api change...should we increment the working version number to a major increment?
Let's add ! in the PR description and then we'll do it at release time
(your approach is reasonable but it may not generalize well to multiple breaking changes in a release without another mechanism...)
@max-sixtymax-sixty
changed the title
refactor: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, deprecate prqlc-astJun 20, 2024
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 02:46
@m-spanm-span
mentioned this pull request
Jun 20, 2024

Closed

@max-sixtymax-sixty
changed the title
refactor!: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, remove prqlc-astJun 20, 2024
@max-sixty
max-sixty
merged commit 6357126
into
PRQL:mainJun 20, 2024
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Thank you @m-span !
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@max-sixtymax-sixtymax-sixty left review comments
@aljazerzenaljazerzenAwaiting requested review from aljazerzen

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@m-span@max-sixty

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
refactor!: prqlc-parser major reorg changes, remove prqlc-ast by m-span · Pull Request #4634 · PRQL/prql · GitHub
Skip to content
refactor!: prqlc-parser major reorg changes, remove prqlc-ast - #4634
Merged
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Jun 20, 2024
Merged
refactor!: prqlc-parser major reorg changes, remove prqlc-ast#4634
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Conversation
@m-span
@m-spanm-span
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
From #4603 , we have the following new terminology:
stagesub-stageAST
parselexerstring -> LR — Lexer Representation
parseparsertokens -> PR — Parser Representation
semanticast_expandPR -> PL — Pipelined Language
semanticresolverPL
semanticflattenPL
semanticloweringPL -> RQ — Resolved Query
sqlpreprocessRQ
sqlsrq-compilerRQ -> PQ — Partitioned Query
sqlpostprocessPQ
sqlsql-compilerPQ -> sqlparser::ast
sqlcodegensqlparser::ast -> string
Goals:
  •  Move all files from prqlc-ast to prqlc-parser. prqlc-ast is to be deprecated
  •  Within parser stage, rename objects, modules and methods to meet new spec. Organize lexer and parser stage into separate modules.
@m-span
m-span
commented
Jun 18, 2024

Copy link


Copy Markdown

CollaboratorAuthor
still a wip, need to make sure all references are updated correctly, but feel free to take a look and comment at how prqlc-parser is now organized
@max-sixty
max-sixty
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Looking great at first glance!
(Unfortunately it looks like it may not be based off the latest main and so have some conflicts; worth pulling the latest main... really hope that's not a painful merge. To the extent it's caused by #4522, there are instructions there for how to reproduce that; so running the same commands on this branch should produce a very similar, easily-mergable result)
@max-sixty
max-sixty
commented
Jun 18, 2024

Copy link


Copy Markdown

Member
Super minor but possibly standards are for pr to be lowercase; no view from me beyond following the rust naming standards
@m-span
m-span
marked this pull request as ready for review
June 20, 2024 00:00
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.

These changes are absolutely Major version number update-API-breaking changes, since we are re-naming our objects.
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lib.rs
Outdated

@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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 Expr etc; so I guess that's possible here?!
(a couple are easy clippy ones, will probably auto-fix with clippy --fix)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
@max-sixty
Remove ast_code_matches file
f3bb696
No longer needed; we're not going to have multiple copies of AST items any longer...
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
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...)
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/error/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/lr/mod.rs
@max-sixtymax-sixtyJun 20, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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...
@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 01:52
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
Looking good from this side. Cleanups were great. Just a small shame I coudln't get the refactor on prqlc_parser::parse_source() working that I had to undo it via this commit.

It did some nice things like hide stuff that didn't need to be exposed
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
I'll add it.

Also, this is a breaking api change...should we increment the working version number to a major increment?
@m-span
m-span
disabled auto-merge
June 20, 2024 02:13
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Also, this is a breaking api change...should we increment the working version number to a major increment?
Let's add ! in the PR description and then we'll do it at release time
(your approach is reasonable but it may not generalize well to multiple breaking changes in a release without another mechanism...)
@max-sixtymax-sixty
changed the title
refactor: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, deprecate prqlc-astJun 20, 2024
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 02:46
@m-spanm-span
mentioned this pull request
Jun 20, 2024

Closed

@max-sixtymax-sixty
changed the title
refactor!: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, remove prqlc-astJun 20, 2024
@max-sixty
max-sixty
merged commit 6357126
into
PRQL:mainJun 20, 2024
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Thank you @m-span !
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@max-sixtymax-sixtymax-sixty left review comments
@aljazerzenaljazerzenAwaiting requested review from aljazerzen

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@m-span@max-sixty

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
refactor!: prqlc-parser major reorg changes, remove prqlc-ast by m-span · Pull Request #4634 · PRQL/prql · GitHub
Skip to content
refactor!: prqlc-parser major reorg changes, remove prqlc-ast - #4634
Merged
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Jun 20, 2024
Merged
refactor!: prqlc-parser major reorg changes, remove prqlc-ast#4634
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Conversation
@m-span
@m-spanm-span
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
From #4603 , we have the following new terminology:
stagesub-stageAST
parselexerstring -> LR — Lexer Representation
parseparsertokens -> PR — Parser Representation
semanticast_expandPR -> PL — Pipelined Language
semanticresolverPL
semanticflattenPL
semanticloweringPL -> RQ — Resolved Query
sqlpreprocessRQ
sqlsrq-compilerRQ -> PQ — Partitioned Query
sqlpostprocessPQ
sqlsql-compilerPQ -> sqlparser::ast
sqlcodegensqlparser::ast -> string
Goals:
  •  Move all files from prqlc-ast to prqlc-parser. prqlc-ast is to be deprecated
  •  Within parser stage, rename objects, modules and methods to meet new spec. Organize lexer and parser stage into separate modules.
@m-span
m-span
commented
Jun 18, 2024

Copy link


Copy Markdown

CollaboratorAuthor
still a wip, need to make sure all references are updated correctly, but feel free to take a look and comment at how prqlc-parser is now organized
@max-sixty
max-sixty
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Looking great at first glance!
(Unfortunately it looks like it may not be based off the latest main and so have some conflicts; worth pulling the latest main... really hope that's not a painful merge. To the extent it's caused by #4522, there are instructions there for how to reproduce that; so running the same commands on this branch should produce a very similar, easily-mergable result)
@max-sixty
max-sixty
commented
Jun 18, 2024

Copy link


Copy Markdown

Member
Super minor but possibly standards are for pr to be lowercase; no view from me beyond following the rust naming standards
@m-span
m-span
marked this pull request as ready for review
June 20, 2024 00:00
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.

These changes are absolutely Major version number update-API-breaking changes, since we are re-naming our objects.
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lib.rs
Outdated

@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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 Expr etc; so I guess that's possible here?!
(a couple are easy clippy ones, will probably auto-fix with clippy --fix)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
@max-sixty
Remove ast_code_matches file
f3bb696
No longer needed; we're not going to have multiple copies of AST items any longer...
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
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...)
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/error/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/lr/mod.rs
@max-sixtymax-sixtyJun 20, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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...
@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 01:52
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
Looking good from this side. Cleanups were great. Just a small shame I coudln't get the refactor on prqlc_parser::parse_source() working that I had to undo it via this commit.

It did some nice things like hide stuff that didn't need to be exposed
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
I'll add it.

Also, this is a breaking api change...should we increment the working version number to a major increment?
@m-span
m-span
disabled auto-merge
June 20, 2024 02:13
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Also, this is a breaking api change...should we increment the working version number to a major increment?
Let's add ! in the PR description and then we'll do it at release time
(your approach is reasonable but it may not generalize well to multiple breaking changes in a release without another mechanism...)
@max-sixtymax-sixty
changed the title
refactor: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, deprecate prqlc-astJun 20, 2024
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 02:46
@m-spanm-span
mentioned this pull request
Jun 20, 2024

Closed

@max-sixtymax-sixty
changed the title
refactor!: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, remove prqlc-astJun 20, 2024
@max-sixty
max-sixty
merged commit 6357126
into
PRQL:mainJun 20, 2024
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Thank you @m-span !
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@max-sixtymax-sixtymax-sixty left review comments
@aljazerzenaljazerzenAwaiting requested review from aljazerzen

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@m-span@max-sixty

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
refactor!: prqlc-parser major reorg changes, remove prqlc-ast by m-span · Pull Request #4634 · PRQL/prql · GitHub
Skip to content
refactor!: prqlc-parser major reorg changes, remove prqlc-ast - #4634
Merged
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Jun 20, 2024
Merged
refactor!: prqlc-parser major reorg changes, remove prqlc-ast#4634
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Conversation
@m-span
@m-spanm-span
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
From #4603 , we have the following new terminology:
stagesub-stageAST
parselexerstring -> LR — Lexer Representation
parseparsertokens -> PR — Parser Representation
semanticast_expandPR -> PL — Pipelined Language
semanticresolverPL
semanticflattenPL
semanticloweringPL -> RQ — Resolved Query
sqlpreprocessRQ
sqlsrq-compilerRQ -> PQ — Partitioned Query
sqlpostprocessPQ
sqlsql-compilerPQ -> sqlparser::ast
sqlcodegensqlparser::ast -> string
Goals:
  •  Move all files from prqlc-ast to prqlc-parser. prqlc-ast is to be deprecated
  •  Within parser stage, rename objects, modules and methods to meet new spec. Organize lexer and parser stage into separate modules.
@m-span
m-span
commented
Jun 18, 2024

Copy link


Copy Markdown

CollaboratorAuthor
still a wip, need to make sure all references are updated correctly, but feel free to take a look and comment at how prqlc-parser is now organized
@max-sixty
max-sixty
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Looking great at first glance!
(Unfortunately it looks like it may not be based off the latest main and so have some conflicts; worth pulling the latest main... really hope that's not a painful merge. To the extent it's caused by #4522, there are instructions there for how to reproduce that; so running the same commands on this branch should produce a very similar, easily-mergable result)
@max-sixty
max-sixty
commented
Jun 18, 2024

Copy link


Copy Markdown

Member
Super minor but possibly standards are for pr to be lowercase; no view from me beyond following the rust naming standards
@m-span
m-span
marked this pull request as ready for review
June 20, 2024 00:00
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.

These changes are absolutely Major version number update-API-breaking changes, since we are re-naming our objects.
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lib.rs
Outdated

@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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 Expr etc; so I guess that's possible here?!
(a couple are easy clippy ones, will probably auto-fix with clippy --fix)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
@max-sixty
Remove ast_code_matches file
f3bb696
No longer needed; we're not going to have multiple copies of AST items any longer...
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
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...)
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/error/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/lr/mod.rs
@max-sixtymax-sixtyJun 20, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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...
@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 01:52
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
Looking good from this side. Cleanups were great. Just a small shame I coudln't get the refactor on prqlc_parser::parse_source() working that I had to undo it via this commit.

It did some nice things like hide stuff that didn't need to be exposed
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
I'll add it.

Also, this is a breaking api change...should we increment the working version number to a major increment?
@m-span
m-span
disabled auto-merge
June 20, 2024 02:13
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Also, this is a breaking api change...should we increment the working version number to a major increment?
Let's add ! in the PR description and then we'll do it at release time
(your approach is reasonable but it may not generalize well to multiple breaking changes in a release without another mechanism...)
@max-sixtymax-sixty
changed the title
refactor: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, deprecate prqlc-astJun 20, 2024
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 02:46
@m-spanm-span
mentioned this pull request
Jun 20, 2024

Closed

@max-sixtymax-sixty
changed the title
refactor!: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, remove prqlc-astJun 20, 2024
@max-sixty
max-sixty
merged commit 6357126
into
PRQL:mainJun 20, 2024
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Thank you @m-span !
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@max-sixtymax-sixtymax-sixty left review comments
@aljazerzenaljazerzenAwaiting requested review from aljazerzen

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@m-span@max-sixty

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
refactor!: prqlc-parser major reorg changes, remove prqlc-ast by m-span · Pull Request #4634 · PRQL/prql · GitHub
Skip to content
refactor!: prqlc-parser major reorg changes, remove prqlc-ast - #4634
Merged
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Jun 20, 2024
Merged
refactor!: prqlc-parser major reorg changes, remove prqlc-ast#4634
max-sixty merged 16 commits into
PRQL:mainfrom 
m-span:renaming
Conversation
@m-span
@m-spanm-span
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
From #4603 , we have the following new terminology:
stagesub-stageAST
parselexerstring -> LR — Lexer Representation
parseparsertokens -> PR — Parser Representation
semanticast_expandPR -> PL — Pipelined Language
semanticresolverPL
semanticflattenPL
semanticloweringPL -> RQ — Resolved Query
sqlpreprocessRQ
sqlsrq-compilerRQ -> PQ — Partitioned Query
sqlpostprocessPQ
sqlsql-compilerPQ -> sqlparser::ast
sqlcodegensqlparser::ast -> string
Goals:
  •  Move all files from prqlc-ast to prqlc-parser. prqlc-ast is to be deprecated
  •  Within parser stage, rename objects, modules and methods to meet new spec. Organize lexer and parser stage into separate modules.
@m-span
m-span
commented
Jun 18, 2024

Copy link


Copy Markdown

CollaboratorAuthor
still a wip, need to make sure all references are updated correctly, but feel free to take a look and comment at how prqlc-parser is now organized
@max-sixty
max-sixty
commented
Jun 18, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Looking great at first glance!
(Unfortunately it looks like it may not be based off the latest main and so have some conflicts; worth pulling the latest main... really hope that's not a painful merge. To the extent it's caused by #4522, there are instructions there for how to reproduce that; so running the same commands on this branch should produce a very similar, easily-mergable result)
@max-sixty
max-sixty
commented
Jun 18, 2024

Copy link


Copy Markdown

Member
Super minor but possibly standards are for pr to be lowercase; no view from me beyond following the rust naming standards
@m-span
m-span
marked this pull request as ready for review
June 20, 2024 00:00
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.

These changes are absolutely Major version number update-API-breaking changes, since we are re-naming our objects.
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
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.
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lib.rs
Outdated

@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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 Expr etc; so I guess that's possible here?!
(a couple are easy clippy ones, will probably auto-fix with clippy --fix)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
@max-sixty
Remove ast_code_matches file
f3bb696
No longer needed; we're not going to have multiple copies of AST items any longer...
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...
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...)
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/parser/pr/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/error/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/mod.rs
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadprqlc/prqlc-parser/src/lexer/lr/mod.rs
@max-sixtymax-sixtyJun 20, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
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...
@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 01:52
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
Looking good from this side. Cleanups were great. Just a small shame I coudln't get the refactor on prqlc_parser::parse_source() working that I had to undo it via this commit.

It did some nice things like hide stuff that didn't need to be exposed
@m-span
m-span
commented
Jun 20, 2024

Copy link


Copy Markdown

CollaboratorAuthor
(could definitely do with a changelog entry, doesn't need to be part of this PR tho)
I'll add it.

Also, this is a breaking api change...should we increment the working version number to a major increment?
@m-span
m-span
disabled auto-merge
June 20, 2024 02:13
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Also, this is a breaking api change...should we increment the working version number to a major increment?
Let's add ! in the PR description and then we'll do it at release time
(your approach is reasonable but it may not generalize well to multiple breaking changes in a release without another mechanism...)
@max-sixtymax-sixty
changed the title
refactor: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, deprecate prqlc-astJun 20, 2024
max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

max-sixty
max-sixty
reviewed
Jun 20, 2024
View reviewed changes
Comment threadCHANGELOG.md
Outdated

@max-sixty
max-sixty
enabled auto-merge (squash)
June 20, 2024 02:46
@m-spanm-span
mentioned this pull request
Jun 20, 2024

Closed

@max-sixtymax-sixty
changed the title
refactor!: prqlc-parser major reorg changes, deprecate prqlc-astrefactor!: prqlc-parser major reorg changes, remove prqlc-astJun 20, 2024
@max-sixty
max-sixty
merged commit 6357126
into
PRQL:mainJun 20, 2024
@max-sixty
max-sixty
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Thank you @m-span !
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@max-sixtymax-sixtymax-sixty left review comments
@aljazerzenaljazerzenAwaiting requested review from aljazerzen

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@m-span@max-sixty