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

feat(rome_service): traverse upwards the fs to discover the config file #4224

Merged
merged 4 commits into from
Feb 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/rome_cli/src/commands/rage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use rome_diagnostics::termcolor::{ColorChoice, WriteColor};
use rome_diagnostics::{termcolor, PrintDescription};
use rome_fs::FileSystem;
use rome_service::workspace::{client, RageEntry, RageParams};
use rome_service::{load_config, BasePath, DynRef, Workspace};
use rome_service::{load_config, ConfigurationBasePath, DynRef, Workspace};
use std::{env, io, ops::Deref};
use tokio::runtime::Runtime;

Expand Down Expand Up @@ -163,7 +163,7 @@ impl Display for RageConfiguration<'_, '_> {
fn fmt(&self, fmt: &mut Formatter) -> io::Result<()> {
Section("Rome Configuration").fmt(fmt)?;

match load_config(self.0, BasePath::default()) {
match load_config(self.0, ConfigurationBasePath::default()) {
Ok(None) => KeyValuePair("Status", markup!(<Dim>"unset"</Dim>)).fmt(fmt)?,
Ok(Some(deserialized)) => {
let (configuration, diagnostics) = deserialized.consume();
Expand Down
6 changes: 3 additions & 3 deletions crates/rome_cli/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::path::PathBuf;

use crate::{CliDiagnostic, CliSession};
use rome_deserialize::Deserialized;
use rome_service::{load_config, BasePath, Configuration};
use rome_service::{load_config, Configuration, ConfigurationBasePath};

/// Load the configuration for this session of the CLI, merging the content of
/// the `rome.json` file if it exists on disk with common command line options
Expand All @@ -15,8 +15,8 @@ pub(crate) fn load_configuration(
.map_err(|source| CliDiagnostic::parse_error("--config-path", source))?;

let base_path = match config_path {
None => BasePath::default(),
Some(path) => BasePath::FromUser(path),
None => ConfigurationBasePath::default(),
Some(path) => ConfigurationBasePath::FromUser(path),
};

let config = load_config(&session.app.fs, base_path)?;
Expand Down
6 changes: 3 additions & 3 deletions crates/rome_lsp/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use rome_console::markup;
use rome_fs::{FileSystem, OsFileSystem, RomePath};
use rome_service::workspace::{FeatureName, PullDiagnosticsParams, SupportsFeatureParams};
use rome_service::workspace::{RageEntry, RageParams, RageResult, UpdateSettingsParams};
use rome_service::{load_config, BasePath, Workspace};
use rome_service::{load_config, ConfigurationBasePath, Workspace};
use rome_service::{DynRef, WorkspaceError};
use serde_json::Value;
use std::collections::HashMap;
Expand Down Expand Up @@ -365,8 +365,8 @@ impl Session {
#[tracing::instrument(level = "debug", skip(self))]
pub(crate) async fn load_workspace_settings(&self) {
let base_path = match self.base_path() {
None => BasePath::default(),
Some(path) => BasePath::Lsp(path),
None => ConfigurationBasePath::default(),
Some(path) => ConfigurationBasePath::Lsp(path),
};

let status = match load_config(&self.fs, base_path) {
Expand Down
119 changes: 79 additions & 40 deletions crates/rome_service/src/configuration/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,14 @@ impl FilesConfiguration {
const KNOWN_KEYS: &'static [&'static str] = &["maxSize", "ignore"];
}

/// - [Result]: if an error occurred while loading the configuration file.
/// - [Option]: sometimes not having a configuration file should not be an error, so we need this type.
/// - [Deserialized]: result of the deserialization of the configuration.
/// - [Configuration]: the type needed to [Deserialized] to infer the return type.
type LoadConfig = Result<Option<Deserialized<Configuration>>, WorkspaceError>;

#[derive(Debug, Default, PartialEq)]
pub enum BasePath {
pub enum ConfigurationBasePath {
/// The default mode, not having a configuration file is not an error.
#[default]
None,
Expand All @@ -129,53 +133,88 @@ pub enum BasePath {
FromUser(PathBuf),
}

impl ConfigurationBasePath {
const fn is_from_user(&self) -> bool {
matches!(self, ConfigurationBasePath::FromUser(_))
}
}

/// Load the configuration from the file system.
///
/// The configuration file will be read from the `file_system`.
/// An optional base path can be appended to the configuration file path.
pub fn load_config(file_system: &DynRef<dyn FileSystem>, base_path: BasePath) -> LoadConfig {
/// The configuration file will be read from the `file_system`. A [base path](ConfigurationBasePath) should be provided.
///
/// The function will try to traverse upwards the file system until if finds a `rome.json` file, or there
/// aren't directories anymore.
///
/// If a the configuration base path was provided by the user, the function will error. If not, Rome will use
/// its defaults.
pub fn load_config(
file_system: &DynRef<dyn FileSystem>,
base_path: ConfigurationBasePath,
) -> LoadConfig {
let config_name = file_system.config_name();
let configuration_path = match base_path {
BasePath::Lsp(ref path) | BasePath::FromUser(ref path) => path.join(config_name),
let mut configuration_path = match base_path {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be helpful for users, if we could show a message when a rome.json configuration file was found using upwards-traversal (auto-search).

Alternatively, we could always show the path of the current rome.json configuration file (if any).

The from_parent variable is a bool primitive.

Suggested change
let mut configuration_path = match base_path {
let mut from_parent = false;
let mut configuration_path = match base_path {

ConfigurationBasePath::Lsp(ref path) | ConfigurationBasePath::FromUser(ref path) => {
path.join(config_name)
}
_ => PathBuf::from(config_name),
};
let should_error = base_path.is_from_user();
ematipico marked this conversation as resolved.
Show resolved Hide resolved
info!(
"Attempting to read the configuration file from {:?}",
configuration_path
"Attempting to read the configuration file from {}",
configuration_path.display()
);
let options = OpenOptions::default().read(true);
let file = file_system.open_with_options(&configuration_path, options);
match file {
Ok(mut file) => {
let mut buffer = String::new();
file.read_to_string(&mut buffer).map_err(|_| {
WorkspaceError::cant_read_file(format!("{}", configuration_path.display()))
})?;

let deserialized = deserialize_from_json::<Configuration>(&buffer)
.with_file_path(&configuration_path.display().to_string());
Ok(Some(deserialized))
}
Err(err) => {
// We skip the error when the configuration file is not found.
// Not having a configuration file is only an error when the `base_path` is
// set to `BasePath::FromUser`.
if match base_path {
BasePath::FromUser(_) => true,
_ => err.kind() != ErrorKind::NotFound,
} {
return Err(WorkspaceError::cant_read_file(format!(
"{}",
configuration_path.display()
)));

let mut from_parent = false;
loop {
let options = OpenOptions::default().read(true);
let file = file_system.open_with_options(&configuration_path, options);
return match file {
Ok(mut file) => {
let mut buffer = String::new();
file.read_to_string(&mut buffer).map_err(|_| {
WorkspaceError::cant_read_file(format!("{}", configuration_path.display()))
})?;

if from_parent {
info!(
"Rome auto discovered a configuration file at following path that wasn't in the working directory: {}",
configuration_path.display()
);
}

let deserialized = deserialize_from_json::<Configuration>(&buffer)
.with_file_path(&configuration_path.display().to_string());
Ok(Some(deserialized))
ematipico marked this conversation as resolved.
Show resolved Hide resolved
}
error!(
"Could not read the configuration file from {:?}, reason:\n {}",
configuration_path.display(),
err
);
Ok(None)
}
Err(err) => {
// base paths from users are not eligible for auto discovery
if !base_path.is_from_user() {
ematipico marked this conversation as resolved.
Show resolved Hide resolved
if let Some(path) = configuration_path.parent() {
if path.is_dir() {
configuration_path = path.join(config_name);
from_parent = true;
continue;
}
}
}
// We skip the error when the configuration file is not found.
// Not having a configuration file is only an error when the `base_path` is
// set to `BasePath::FromUser`.
if should_error || err.kind() != ErrorKind::NotFound {
ematipico marked this conversation as resolved.
Show resolved Hide resolved
return Err(WorkspaceError::cant_read_file(format!(
"{}",
configuration_path.display()
)));
}
error!(
"Could not read the configuration file from {:?}, reason:\n {}",
configuration_path.display(),
err
);
Ok(None)
}
};
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/rome_service/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ mod diagnostics;
pub mod workspace_types;

pub use crate::configuration::{
create_config, load_config, BasePath, Configuration, ConfigurationDiagnostic,
create_config, load_config, Configuration, ConfigurationBasePath, ConfigurationDiagnostic,
RuleConfiguration, Rules,
};
pub use crate::matcher::{MatchOptions, Matcher, Pattern};
Expand Down
2 changes: 1 addition & 1 deletion website/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"start": "astro dev",
"format": "cargo rome-cli-dev format --write .",
"tsc": "tsc --skipLibCheck",
"check": "cd ../ && cargo rome-cli-dev check ./website/src",
"check": "cargo rome-cli-dev check ./src",
"start:playground": "pnpm build:wasm-dev && pnpm start",
"build": "pnpm build:wasm && pnpm build:js",
"build:js": "astro build",
Expand Down