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

fix(rome_cli): correctly compute configuration #3176

Merged
merged 2 commits into from
Sep 8, 2022
Merged

fix(rome_cli): correctly compute configuration #3176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce66ebd
fix(rome_cli): incorrectly compute configuration
ematipico Sep 7, 2022
26cb57b
chore: add additional test
ematipico Sep 8, 2022
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
15 changes: 5 additions & 10 deletions crates/rome_cli/src/commands/check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,11 @@ use crate::commands::format::apply_format_settings_from_cli;
use crate::{execute_mode, CliSession, Execution, Termination, TraversalMode};
use rome_diagnostics::MAXIMUM_DISPLAYABLE_DIAGNOSTICS;
use rome_service::load_config;
use rome_service::settings::WorkspaceSettings;
use rome_service::workspace::{FixFileMode, UpdateSettingsParams};

/// Handler for the "check" command of the Rome CLI
pub(crate) fn check(mut session: CliSession) -> Result<(), Termination> {
let configuration = load_config(&session.app.fs, None)?;
let mut workspace_settings = WorkspaceSettings::default();

let max_diagnostics: Option<u16> = session
.args
.opt_value_from_str("--max-diagnostics")
Expand All @@ -31,13 +28,11 @@ pub(crate) fn check(mut session: CliSession) -> Result<(), Termination> {
20
};

if let Some(configuration) = configuration {
session
.app
.workspace
.update_settings(UpdateSettingsParams { configuration })?;
}
apply_format_settings_from_cli(&mut session, &mut workspace_settings)?;
let configuration = apply_format_settings_from_cli(&mut session, configuration)?;
session
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this change mean for concurrent usages of the deamon. Do the CLI argument change for all requests to the workspace?

Copy link
Contributor Author

@ematipico ematipico Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure want you mean. We only update the settings once (request), then each format_file request (for example) reads those settings anytime they need.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but what if multiple clients are connected to the same daemon

# client 1
rome format --indent-style="tab" --use-server

#client 2
rome format --indent-style="space" --use-server

And both commands run at the same time. Which configuration would be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both clients have two different instances of Workspace, so their configuration won't mix up.

.app
.workspace
.update_settings(UpdateSettingsParams { configuration })?;

let apply = session.args.contains("--apply");
let apply_suggested = session.args.contains("--apply-suggested");
Expand Down
14 changes: 5 additions & 9 deletions crates/rome_cli/src/commands/ci.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,18 @@
use crate::{execute_mode, CliSession, Execution, Termination, TraversalMode};
use rome_service::load_config;
use rome_service::settings::WorkspaceSettings;
use rome_service::workspace::UpdateSettingsParams;

use super::format::apply_format_settings_from_cli;

/// Handler for the "ci" command of the Rome CLI
pub(crate) fn ci(mut session: CliSession) -> Result<(), Termination> {
let configuration = load_config(&session.app.fs, None)?;
let mut workspace_settings = WorkspaceSettings::default();
let configuration = apply_format_settings_from_cli(&mut session, configuration)?;

if let Some(configuration) = configuration {
session
.app
.workspace
.update_settings(UpdateSettingsParams { configuration })?;
}
apply_format_settings_from_cli(&mut session, &mut workspace_settings)?;
session
.app
.workspace
.update_settings(UpdateSettingsParams { configuration })?;

execute_mode(Execution::new(TraversalMode::CI), session)
}
99 changes: 57 additions & 42 deletions crates/rome_cli/src/commands/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
use rome_formatter::IndentStyle;
use rome_service::{load_config, settings::WorkspaceSettings, workspace::UpdateSettingsParams};
use rome_service::configuration::{
FormatterConfiguration, JavascriptConfiguration, JavascriptFormatter, PlainIndentStyle,
};
use rome_service::{load_config, workspace::UpdateSettingsParams, Configuration};
use std::path::PathBuf;

use crate::execute::ReportMode;
Expand All @@ -8,16 +11,12 @@ use crate::{execute_mode, CliSession, Execution, Termination, TraversalMode};
/// Handler for the "format" command of the Rome CLI
pub(crate) fn format(mut session: CliSession) -> Result<(), Termination> {
let configuration = load_config(&session.app.fs, None)?;
let mut workspace_settings = WorkspaceSettings::default();
let configuration = apply_format_settings_from_cli(&mut session, configuration)?;

if let Some(configuration) = configuration {
session
.app
.workspace
.update_settings(UpdateSettingsParams { configuration })?;
}

apply_format_settings_from_cli(&mut session, &mut workspace_settings)?;
session
.app
.workspace
.update_settings(UpdateSettingsParams { configuration })?;

let is_write = session.args.contains("--write");
let ignore_errors = session.args.contains("--skip-errors");
Expand Down Expand Up @@ -67,8 +66,21 @@ pub(crate) fn format(mut session: CliSession) -> Result<(), Termination> {
/// into the workspace settings
pub(crate) fn apply_format_settings_from_cli(
session: &mut CliSession,
workspace_settings: &mut WorkspaceSettings,
) -> Result<(), Termination> {
configuration: Option<Configuration>,
) -> Result<Configuration, Termination> {
let mut configuration = if let Some(configuration) = configuration {
configuration
} else {
Configuration {
formatter: Some(FormatterConfiguration::default()),
javascript: Some(JavascriptConfiguration {
formatter: Some(JavascriptFormatter::default()),
globals: None,
}),
..Configuration::default()
}
};

let size = session
.args
.opt_value_from_str("--indent-size")
Expand All @@ -85,27 +97,29 @@ pub(crate) fn apply_format_settings_from_cli(
source,
})?;

match indent_style {
Some(IndentStyle::Tab) => {
workspace_settings.formatter.indent_style = Some(IndentStyle::Tab);
}
Some(IndentStyle::Space(default_size)) => {
workspace_settings.formatter.indent_style =
Some(IndentStyle::Space(size.unwrap_or(default_size)));
}
None => {}
}

let quote_style = session
let line_width = session
.args
.opt_value_from_str("--quote-style")
.opt_value_from_str("--line-width")
.map_err(|source| Termination::ParseError {
argument: "--quote-style",
argument: "--line-width",
source,
})?;

if let Some(quote_style) = quote_style {
workspace_settings.languages.javascript.format.quote_style = Some(quote_style);
if let Some(formatter) = configuration.formatter.as_mut() {
match indent_style {
Some(IndentStyle::Tab) => {
formatter.indent_style = PlainIndentStyle::Tab;
}
Some(IndentStyle::Space(default_size)) => {
formatter.indent_style = PlainIndentStyle::Space;
formatter.indent_size = size.unwrap_or(default_size);
}
None => {}
}

if let Some(line_width) = line_width {
formatter.line_width = line_width;
}
}

let quote_properties = session
Expand All @@ -116,25 +130,26 @@ pub(crate) fn apply_format_settings_from_cli(
source,
})?;

if let Some(quote_properties) = quote_properties {
workspace_settings
.languages
.javascript
.format
.quote_properties = Some(quote_properties);
}

let line_width = session
let quote_style = session
.args
.opt_value_from_str("--line-width")
.opt_value_from_str("--quote-style")
.map_err(|source| Termination::ParseError {
argument: "--line-width",
argument: "--quote-style",
source,
})?;
if let Some(javascript) = configuration
.javascript
.as_mut()
.and_then(|j| j.formatter.as_mut())
{
if let Some(quote_properties) = quote_properties {
javascript.quote_properties = quote_properties;
}

if let Some(line_width) = line_width {
workspace_settings.formatter.line_width = Some(line_width);
if let Some(quote_style) = quote_style {
javascript.quote_style = quote_style;
}
}

Ok(())
Ok(configuration)
}
147 changes: 147 additions & 0 deletions crates/rome_cli/tests/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,27 @@ const NO_DEAD_CODE_ERROR: &str = r#"function f() {
}
"#;

const APPLY_QUOTE_STYLE_BEFORE: &str = r#"
let a = "something";
let b = {
"hey": "hello"
};"#;

const APPLY_QUOTE_STYLE_AFTER: &str = "let a = 'something';
let b = {\n\t'hey': 'hello',\n};\n";

const CUSTOM_CONFIGURATION_BEFORE: &str = r#"function f() {
return { a, b }
}"#;

const CUSTOM_CONFIGURATION_AFTER: &str = "function f() {
return {
a,
b,
};
}
";

mod check {
use super::*;
use crate::configs::{
Expand Down Expand Up @@ -758,6 +779,132 @@ mod format {
assert_cli_snapshot(module_path!(), "formatter_lint_warning", fs, console);
}

#[test]
fn applies_custom_configuration() {
let mut fs = MemoryFileSystem::default();
let mut console = BufferConsole::default();

let file_path = Path::new("file.js");
fs.insert(file_path.into(), CUSTOM_CONFIGURATION_BEFORE.as_bytes());

let result = run_cli(
DynRef::Borrowed(&mut fs),
DynRef::Borrowed(&mut console),
Arguments::from_vec(vec![
OsString::from("format"),
OsString::from("--line-width"),
OsString::from("10"),
OsString::from("--indent-style"),
OsString::from("space"),
OsString::from("--indent-size"),
OsString::from("8"),
OsString::from("--write"),
file_path.as_os_str().into(),
]),
);

assert!(result.is_ok(), "run_cli returned {result:?}");

let mut file = fs
.open(file_path)
.expect("formatting target file was removed by the CLI");

let mut content = String::new();
file.read_to_string(&mut content)
.expect("failed to read file from memory FS");

assert_eq!(content, CUSTOM_CONFIGURATION_AFTER);

drop(file);
assert_cli_snapshot(module_path!(), "applies_custom_configuration", fs, console);
}

#[test]
fn applies_custom_configuration_over_config_file() {
let mut fs = MemoryFileSystem::default();
let mut console = BufferConsole::default();

let file_path = Path::new("rome.json");
fs.insert(file_path.into(), CONFIG_FORMAT.as_bytes());

let file_path = Path::new("file.js");
fs.insert(file_path.into(), CUSTOM_CONFIGURATION_BEFORE.as_bytes());

let result = run_cli(
DynRef::Borrowed(&mut fs),
DynRef::Borrowed(&mut console),
Arguments::from_vec(vec![
OsString::from("format"),
OsString::from("--line-width"),
OsString::from("10"),
OsString::from("--indent-style"),
OsString::from("space"),
OsString::from("--indent-size"),
OsString::from("8"),
OsString::from("--write"),
file_path.as_os_str().into(),
]),
);

assert!(result.is_ok(), "run_cli returned {result:?}");

let mut file = fs
.open(file_path)
.expect("formatting target file was removed by the CLI");

let mut content = String::new();
file.read_to_string(&mut content)
.expect("failed to read file from memory FS");

assert_eq!(content, CUSTOM_CONFIGURATION_AFTER);

drop(file);
assert_cli_snapshot(
module_path!(),
"applies_custom_configuration_over_config_file",
fs,
console,
);
}

#[test]
fn applies_custom_quote_style() {
let mut fs = MemoryFileSystem::default();
let mut console = BufferConsole::default();

let file_path = Path::new("file.js");
fs.insert(file_path.into(), APPLY_QUOTE_STYLE_BEFORE.as_bytes());

let result = run_cli(
DynRef::Borrowed(&mut fs),
DynRef::Borrowed(&mut console),
Arguments::from_vec(vec![
OsString::from("format"),
OsString::from("--quote-style"),
OsString::from("single"),
OsString::from("--quote-properties"),
OsString::from("preserve"),
OsString::from("--write"),
file_path.as_os_str().into(),
]),
);

assert!(result.is_ok(), "run_cli returned {result:?}");

let mut file = fs
.open(file_path)
.expect("formatting target file was removed by the CLI");

let mut content = String::new();
file.read_to_string(&mut content)
.expect("failed to read file from memory FS");

assert_eq!(content, APPLY_QUOTE_STYLE_AFTER);

drop(file);
assert_cli_snapshot(module_path!(), "applies_custom_quote_style", fs, console);
}

#[test]
fn indent_style_parse_errors() {
let mut console = BufferConsole::default();
Expand Down
19 changes: 19 additions & 0 deletions crates/rome_cli/tests/snapshots/main_format/applies_custom_configuration.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
source: crates/rome_cli/tests/snap_test.rs
expression: content
---
## `file.js`

```js
function f() {
return {
a,
b,
};
}

```

# Emitted Messages


Loading