- Notifications
You must be signed in to change notification settings - Fork102
refactor: drop change.rs <3#447
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
very nice! much simpler, and I love that there's no separateParsedDocument anymore :)
| /// Creates a child statement ID with the given content and parent content. | ||
| pub fn new_child(child_content: &str, parent_content: &str) -> Self { | ||
| StatementId::Child { | ||
| content: child_content.into(), | ||
| parent_content: parent_content.into(), | ||
| } |
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.
Is this used/needed? feels like we could create orphaned childs with this – not a problem right now, but maybe leads to confusion in the future?
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.
nope, thanks!
| } | ||
| } | ||
| pub fnparent(&self) ->Option<StatementId> { | ||
| pub fncontent(&self) ->&str { |
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.
love it
| @@ -641,9 +637,9 @@ impl Workspace for WorkspaceServer { | |||
| }); | |||
| tracing::debug!( | |||
| "Found {} completion items for statement with id {}", | |||
| "Found {} completion items for statement with id {:?}", | |||
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.
that's probably a lot of log output – not sure we even need the print
crates/pgt_lsp/src/capabilities.rs Outdated
| /// Instead, we manually list the command IDs we want to register. | ||
| fn available_command_ids() -> Vec<String> { | ||
| vec
juleswritescodeJul 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Also, we're now just sending the full statement over to the client anyway, so we might as well change the CommandActionCategory::ExecuteStatement(_) to hold aString instead of aStatementId – I think you can then use strum again :)
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.
ah good point!
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.
reverted my changes. I think its simpler to just add a comment to the default implementation
27b6e7f intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
as per PR title. notable changes:
change.rsin favour of a simpleupdate_documentapi that replaces the contentStatementIdto just be a simple wrapper around the actual statement string. The content is wrapped in anArc<str>to make cloning cheap. turns out string interning only makes sense when we have a lot of duplicate strings.Arc<str>is much more efficient and simpler in our case.document.rsand merge it intoparsed_documentto then rename it toDocumentstrumusage to get the available command because it didn't feel right to make an empty string the default for a StatementId. replaced it with a simple manual implementation.I ran a few benchmarks and the statement splitter performance seems to be good enough to run it on every keystroke on the entire file.
ToDo