- Notifications
You must be signed in to change notification settings - Fork102
feat: workspace support#408
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
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Ok(SchemaCacheHandle::new(&self.inner)) | ||
} | ||
} | ||
let schema_cache = Arc::new(run_async(async move { SchemaCache::load(&pool).await })??); |
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.
won't there be race conditions ifload
is called multiple times in parallel and there is no schema_cache in cache? We wouldSchemaCache::load
a couple of times, right?
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.
good catch!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
let settings = self.workspaces(); | ||
// TODO: not sure how we should handle this | ||
// maybe fallback to default settings? or return an error? | ||
let settings = settings.settings().expect("Settings should be initialized"); |
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 there state where a user requests diagnostics before or while settings are set up, for example when the file opens?
If so, I think we should return an emtpy response. But if not, it's fine toexpect
– we do expect the settings to be present for a working LSP right?
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.
yeah, makes sense. added that.
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.
Pull Request Overview
This PR adds workspace folder management andextends
support in configuration loading, ensuring per-connection caches and idle timeouts, and updates LSP/CLI integrations.
- Introduce
register_project_folder
/unregister_project_folder
API and per-project data storage - Implement
extends
logic inPartialConfiguration
with file resolution and merging - Wire workspace registration in LSP and CLI sessions and update file system resolver integration
Reviewed Changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
crates/pgt_workspace/src/workspace.rs | Add project registration/unregistration and slotmap-based storage |
crates/pgt_workspace/src/settings.rs | Manage per-project settings and current-project logic |
crates/pgt_workspace/src/lib.rs | Re-export newPartialConfigurationExt trait |
crates/pgt_workspace/src/diagnostics.rs | BridgeCantLoadExtendFile intoWorkspaceError |
crates/pgt_workspace/src/configuration.rs | Replace payload conversion withtry_from_payload and implementextends merging |
crates/pgt_workspace/Cargo.toml | Addslotmap dependency for project keys |
crates/pgt_lsp/src/session.rs | Introduce client info, callregister_project_folder during initialization |
crates/pgt_lsp/src/server.rs | Handle workspace folder changes and register/unregister methods |
crates/pgt_fs/src/fs/os.rs | Integrateoxc_resolver for config resolution |
crates/pgt_fs/src/fs/memory.rs | Add stub forresolve_configuration in memory FS |
crates/pgt_fs/src/fs.rs | ExtendFileSystem trait withresolve_configuration |
crates/pgt_fs/Cargo.toml | Addoxc_resolver dependency |
crates/pgt_diagnostics/src/adapters.rs | Wrap resolver errors into diagnostics |
crates/pgt_diagnostics/Cargo.toml | Addoxc_resolver dependency |
crates/pgt_configuration/src/lib.rs | Addextends field and default in partial config |
crates/pgt_configuration/src/diagnostics.rs | New diagnostics for extend resolution and load errors |
crates/pgt_configuration/Cargo.toml | Addoxc_resolver dependency |
crates/pgt_cli/src/diagnostics.rs | Update expected diagnostic size |
crates/pgt_cli/src/commands/mod.rs | Register workspace folder in CLI commands |
Cargo.toml | Addoxc_resolver andslotmap to workspace |
Comments suppressed due to low confidence (2)
crates/pgt_workspace/src/configuration.rs:349
- The detection logic treats only
.jsonc
as local files;.json
entries will be resolved externally. Consider includingb"json"
in the match to handle.json
extends as local paths.
if extend_entry_as_path.starts_with(".") || matches!(extend_entry_as_path.extension().map(OsStr::as_encoded_bytes), Some(b"jsonc")) {
crates/pgt_lsp/src/session.rs:492
- This call to
.await
is inside a synchronous function, which will fail to compile. Either makeinitialize
async or spawn a separate async task to log the message.
self.client.log_message(MessageType::ERROR, &error).await;
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.
Beautiful!!!
3698af5
intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
adds workspace support as well as support for
extends
in the config. This now works:./postgrestools.jsonc
{// ...my linter config}
./project_one/postgrestools.jsonc
Will need to check if client-side changes are required to enable workspace support there.
A few internal changes:
ToDo:
try_from_payload
with extendscloses#334