-
Notifications
You must be signed in to change notification settings - Fork 102
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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 if load
is called multiple times in parallel and there is no schema_cache in cache? We would SchemaCache::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!
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 to expect
– 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 and extends
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 new PartialConfigurationExt trait |
crates/pgt_workspace/src/diagnostics.rs | Bridge CantLoadExtendFile into WorkspaceError |
crates/pgt_workspace/src/configuration.rs | Replace payload conversion with try_from_payload and implement extends merging |
crates/pgt_workspace/Cargo.toml | Add slotmap dependency for project keys |
crates/pgt_lsp/src/session.rs | Introduce client info, call register_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 | Integrate oxc_resolver for config resolution |
crates/pgt_fs/src/fs/memory.rs | Add stub for resolve_configuration in memory FS |
crates/pgt_fs/src/fs.rs | Extend FileSystem trait with resolve_configuration |
crates/pgt_fs/Cargo.toml | Add oxc_resolver dependency |
crates/pgt_diagnostics/src/adapters.rs | Wrap resolver errors into diagnostics |
crates/pgt_diagnostics/Cargo.toml | Add oxc_resolver dependency |
crates/pgt_configuration/src/lib.rs | Add extends 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 | Add oxc_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 | Add oxc_resolver and slotmap 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;
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!!!
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