Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Commit

Permalink
feat(editors/vscode): Add requiresConfiguration option (#4023)
Browse files Browse the repository at this point in the history
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Co-authored-by: l3ops <leo@rome.tools>
  • Loading branch information
3 people authored Dec 23, 2022
1 parent c4eb1d5 commit 846ac95
Show file tree
Hide file tree
Showing 8 changed files with 275 additions and 109 deletions.
10 changes: 2 additions & 8 deletions crates/rome_lsp/src/capabilities.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use tower_lsp::lsp_types::{
CodeActionProviderCapability, DocumentOnTypeFormattingOptions, OneOf, ServerCapabilities,
TextDocumentSyncCapability, TextDocumentSyncKind,
CodeActionProviderCapability, ServerCapabilities, TextDocumentSyncCapability,
TextDocumentSyncKind,
};

/// The capabilities to send from server as part of [`InitializeResult`]
Expand All @@ -12,12 +12,6 @@ pub(crate) fn server_capabilities() -> ServerCapabilities {
TextDocumentSyncKind::INCREMENTAL,
)),
code_action_provider: Some(CodeActionProviderCapability::Simple(true)),
document_formatting_provider: Some(OneOf::Left(true)),
document_range_formatting_provider: Some(OneOf::Left(true)),
document_on_type_formatting_provider: Some(DocumentOnTypeFormattingOptions {
first_trigger_character: String::from("}"),
more_trigger_character: Some(vec![String::from("]"), String::from(")")]),
}),
rename_provider: None,
..Default::default()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,18 @@ pub struct WorkspaceSettings {

/// Enable rename capability
pub rename: Option<bool>,

/// Only run Rome if a `rome.json` configuration file exists.
pub require_configuration: Option<bool>,
}

/// The `rome.*` extension settings
#[derive(Debug)]
pub(crate) struct Config {
pub(crate) struct ExtensionSettings {
pub(crate) settings: WorkspaceSettings,
}

impl Config {
impl ExtensionSettings {

This comment has been minimized.

Copy link
@rchl

rchl Dec 29, 2022

From the naming perspective, I find it weird to use "ExtensionSettings" in the server because LSP server is completely independent entity from the VSCode extension. Other editors can run the server and not use any sort of extension.

As long as this naming doesn't leak into public facing documentation then it's probably OK but still feels wrong IMO.

This comment has been minimized.

Copy link
@ematipico

ematipico Dec 29, 2022

Author Contributor

Your argument is entirely valid; it's worth providing an alternative to the naming. Otherwise, your comment isn't actionable and doesn't provide the intended value.

This comment has been minimized.

Copy link
@rchl

rchl Dec 29, 2022

In the spec those are referred to using generic "configuration settings".
Server can retrieve those using workspace/configuration request so that also suggests that those could be called "WorkspaceConfiguration" or similar.

My point was not to provide specific suggestion though as that can be subjective but just point out that referring to "extension" is not appropriate in this context..

pub(crate) fn new() -> Self {
Self {
settings: WorkspaceSettings::default(),
Expand All @@ -37,4 +41,8 @@ impl Config {
);
Ok(())
}

pub(crate) fn requires_configuration(&self) -> bool {
self.settings.require_configuration.unwrap_or_default()
}
}
4 changes: 2 additions & 2 deletions crates/rome_lsp/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
mod capabilities;
mod config;
mod documents;
mod extension_settings;
mod handlers;
mod line_index;
mod requests;
Expand All @@ -9,5 +9,5 @@ mod session;
mod url_interner;
mod utils;

pub use crate::config::WorkspaceSettings;
pub use crate::extension_settings::WorkspaceSettings;
pub use crate::server::{LSPServer, ServerConnection, ServerFactory};
148 changes: 80 additions & 68 deletions crates/rome_lsp/src/server.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use crate::capabilities::server_capabilities;
use crate::requests::syntax_tree::{SyntaxTreePayload, SYNTAX_TREE_REQUEST};
use crate::session::{ClientInformation, Session, SessionHandle, SessionKey};
use crate::session::{
CapabilitySet, CapabilityStatus, ClientInformation, Session, SessionHandle, SessionKey,
};
use crate::utils::{into_lsp_error, panic_to_lsp_error};
use crate::{handlers, requests};
use futures::future::ready;
Expand All @@ -9,6 +11,7 @@ use rome_console::markup;
use rome_fs::CONFIG_NAME;
use rome_service::workspace::{RageEntry, RageParams, RageResult};
use rome_service::{workspace, Workspace};
use serde_json::json;
use std::collections::HashMap;
use std::panic::RefUnwindSafe;
use std::sync::atomic::{AtomicBool, AtomicU64, Ordering};
Expand Down Expand Up @@ -109,78 +112,82 @@ impl LSPServer {
Ok(RageResult { entries })
}

async fn register_capability(&self, registration: Registration) {
let method = registration.method.clone();
async fn setup_capabilities(&self) {
let mut capabilities = CapabilitySet::default();

if let Err(e) = self
.session
.client
.register_capability(vec![registration])
.await
{
error!("Error registering {:?} capability: {}", method, e);
}
}
capabilities.add_capability(
"rome_did_change_extension_settings",
"workspace/didChangeConfiguration",
if self.session.can_register_did_change_configuration() {
CapabilityStatus::Enable(None)
} else {
CapabilityStatus::Disable
},
);

async fn unregister_capability(&self, unregistration: Unregistration) {
let method = unregistration.method.clone();
capabilities.add_capability(
"rome_did_change_workspace_settings",
"workspace/didChangeWatchedFiles",
if let Some(base_path) = self.session.base_path() {
CapabilityStatus::Enable(Some(json!(DidChangeWatchedFilesRegistrationOptions {
watchers: vec![FileSystemWatcher {
glob_pattern: format!("{}/rome.json", base_path.display()),

This comment has been minimized.

Copy link
@rchl

rchl Dec 29, 2022

This probably could be simplified to /rome.json or maybe **/rome.json (would have to be tested) since the pattern is per spec relative to the workspace root.

kind: Some(WatchKind::all()),
}],
})))
} else {
CapabilityStatus::Disable
},
);

if let Err(e) = self
.session
.client
.unregister_capability(vec![unregistration])
.await
{
error!("Error unregistering {:?} capability: {}", method, e);
}
}
capabilities.add_capability(
"rome_formatting",
"textDocument/formatting",
if self.session.is_linting_and_formatting_disabled() {
CapabilityStatus::Disable
} else {
CapabilityStatus::Enable(None)
},
);
capabilities.add_capability(
"rome_range_formatting",
"textDocument/rangeFormatting",
if self.session.is_linting_and_formatting_disabled() {
CapabilityStatus::Disable
} else {
CapabilityStatus::Enable(None)
},
);
capabilities.add_capability(
"rome_on_type_formatting",
"textDocument/onTypeFormatting",
if self.session.is_linting_and_formatting_disabled() {
CapabilityStatus::Disable
} else {
CapabilityStatus::Enable(Some(json!(DocumentOnTypeFormattingRegistrationOptions {
document_selector: None,
first_trigger_character: String::from("}"),
more_trigger_character: Some(vec![String::from("]"), String::from(")")]),
})))
},
);

async fn setup_capabilities(&self) {
let rename = {
let config = self.session.config.read().ok();
let config = self.session.extension_settings.read().ok();
config.and_then(|x| x.settings.rename).unwrap_or(false)
};

if self.session.can_register_did_change_configuration() {
self.register_capability(Registration {
id: "workspace/didChangeConfiguration".to_string(),
method: "workspace/didChangeConfiguration".to_string(),
register_options: None,
})
.await;
}

let base_path = self.session.base_path();

if let Some(base_path) = base_path {
let registration_options = DidChangeWatchedFilesRegistrationOptions {
watchers: vec![FileSystemWatcher {
glob_pattern: format!("{}/rome.json", base_path.display()),
kind: Some(WatchKind::all()),
}],
};
self.register_capability(Registration {
id: "workspace/didChangeWatchedFiles".to_string(),
method: "workspace/didChangeWatchedFiles".to_string(),
register_options: Some(serde_json::to_value(registration_options).unwrap()),
})
.await;
}
capabilities.add_capability(
"rome_rename",
"textDocument/rename",
if rename {
CapabilityStatus::Enable(None)
} else {
CapabilityStatus::Disable
},
);

if rename {
self.register_capability(Registration {
id: "textDocument/rename".to_string(),
method: "textDocument/rename".to_string(),
register_options: None,
})
.await;
} else {
self.unregister_capability(Unregistration {
id: "textDocument/rename".to_string(),
method: "textDocument/rename".to_string(),
})
.await;
}
self.session.register_capabilities(capabilities).await;
}
}

Expand Down Expand Up @@ -237,8 +244,10 @@ impl LanguageServer for LSPServer {

info!("Attempting to load the configuration from 'rome.json' file");

self.session.load_client_configuration().await;
self.session.load_workspace_settings().await;
futures::join!(
self.session.load_extension_settings(),
self.session.load_workspace_settings()
);

let msg = format!("Server initialized with PID: {}", std::process::id());
self.session
Expand All @@ -256,11 +265,13 @@ impl LanguageServer for LSPServer {
Ok(())
}

/// Called when the user changed the editor settings.
#[tracing::instrument(level = "debug", skip(self))]
async fn did_change_configuration(&self, params: DidChangeConfigurationParams) {
let _ = params;
self.session.load_client_configuration().await;
self.session.load_extension_settings().await;
self.setup_capabilities().await;
self.session.update_all_diagnostics().await;
}

#[tracing::instrument(level = "debug", skip(self))]
Expand All @@ -278,6 +289,7 @@ impl LanguageServer for LSPServer {
if let Ok(possible_rome_json) = possible_rome_json {
if possible_rome_json.display().to_string() == CONFIG_NAME {
self.session.load_workspace_settings().await;
self.setup_capabilities().await;
self.session.update_all_diagnostics().await;
// for now we are only interested to the configuration file,
// so it's OK to exist the loop
Expand Down Expand Up @@ -353,7 +365,7 @@ impl LanguageServer for LSPServer {
rome_diagnostics::panic::catch_unwind(move || {
let rename_enabled = self
.session
.config
.extension_settings
.read()
.ok()
.and_then(|config| config.settings.rename)
Expand Down
Loading

0 comments on commit 846ac95

Please sign in to comment.