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

Commit

Permalink
fix(rome_cli): correctly account for diff diagnostics in the printed …
Browse files Browse the repository at this point in the history 
…diagnostics count (#3595)

* fix(rome_cli): correctly account for diff diagnostics in the printed diagnostics count

* add tests
  • Loading branch information
@leops
leops committed Nov 9, 2022
1 parent 903a3ed commit 50dbe68
Show file tree
Hide file tree
Showing 7 changed files with 248 additions and 73 deletions.
28 changes: 1 addition & 27 deletions crates/rome_cli/src/commands/check.rs
View file
Original file line number Diff line number Diff line change
@@ -1,35 +1,12 @@
use crate::commands::format::apply_format_settings_from_cli;
use crate::configuration::load_configuration;
use crate::{execute_mode, CliSession, Execution, Termination, TraversalMode};
use rome_diagnostics::MAXIMUM_DISPLAYABLE_DIAGNOSTICS;
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 mut configuration = load_configuration(&mut session)?;

let max_diagnostics: Option<u16> = session
.args
.opt_value_from_str("--max-diagnostics")
.map_err(|source| Termination::ParseError {
argument: "--max-diagnostics",
source,
})?;

let max_diagnostics = if let Some(max_diagnostics) = max_diagnostics {
if max_diagnostics > MAXIMUM_DISPLAYABLE_DIAGNOSTICS {
return Err(Termination::OverflowNumberArgument(
"--max-diagnostics",
MAXIMUM_DISPLAYABLE_DIAGNOSTICS,
));
}

max_diagnostics
} else {
// default value
20
};

apply_format_settings_from_cli(&mut session, &mut configuration)?;

session
Expand All @@ -54,10 +31,7 @@ pub(crate) fn check(mut session: CliSession) -> Result<(), Termination> {
};

execute_mode(
Execution::new(TraversalMode::Check {
max_diagnostics,
fix_file_mode,
}),
Execution::new(TraversalMode::Check { fix_file_mode }),
session,
)
}
6 changes: 4 additions & 2 deletions crates/rome_cli/src/commands/help.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ const CI: Markup = markup! {
"<Emphasis>"OPTIONS:"</Emphasis>"
"<Dim>"--formatter-enabled"</Dim>" Allow to enable or disable the formatter check. (default: true)
"<Dim>"--linter-enabled"</Dim>" Allow to enable or disable the linter check. (default: true)"
"<Dim>"--linter-enabled"</Dim>" Allow to enable or disable the linter check. (default: true)
"<Dim>"--max-diagnostics"</Dim>" Cap the amount of diagnostics displayed (default: 50)"
{FORMAT_OPTIONS}
};

Expand All @@ -75,7 +76,8 @@ const FORMAT: Markup = markup! {
"<Emphasis>"OPTIONS:"</Emphasis>"
"<Dim>"--write"</Dim>" Edit the files in place (beware!) instead of printing the diff to the console
"<Dim>"--skip-errors"</Dim>" Skip over files containing syntax errors instead of emitting an error diagnostic."
"<Dim>"--skip-errors"</Dim>" Skip over files containing syntax errors instead of emitting an error diagnostic.
"<Dim>"--max-diagnostics"</Dim>" Cap the amount of diagnostics displayed (default: 50)"
{FORMAT_OPTIONS}
""<Dim>"--stdin-file-path <string>"</Dim>" A file name with its extension to pass when reading from standard in, e.g. echo 'let a;' | rome format --stdin-file-path file.js
"
Expand Down
49 changes: 38 additions & 11 deletions crates/rome_cli/src/execute.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::traversal::traverse;
use crate::{CliSession, Termination};
use rome_console::{markup, ConsoleExt};
use rome_diagnostics::file::FileId;
use rome_diagnostics::MAXIMUM_DISPLAYABLE_DIAGNOSTICS;
use rome_fs::RomePath;
use rome_service::workspace::{
FeatureName, FixFileMode, FormatFileParams, Language, OpenFileParams, SupportsFeatureParams,
Expand All @@ -15,14 +16,14 @@ pub(crate) struct Execution {

/// The modality of execution of the traversal
traversal_mode: TraversalMode,

/// The maximum number of diagnostics that can be printed in console
max_diagnostics: u16,
}

pub(crate) enum TraversalMode {
/// This mode is enabled when running the command `rome check`
Check {
/// The maximum number of diagnostics that can be printed in console
max_diagnostics: u16,

/// The type of fixes that should be applied when analyzing a file.
///
/// It's [None] if the `check` command is called without `--apply` or `--apply-suggested`
Expand Down Expand Up @@ -59,6 +60,7 @@ impl Execution {
Self {
report_mode: ReportMode::default(),
traversal_mode: mode,
max_diagnostics: MAXIMUM_DISPLAYABLE_DIAGNOSTICS,
}
}

Expand All @@ -67,6 +69,7 @@ impl Execution {
Self {
traversal_mode,
report_mode,
max_diagnostics: MAXIMUM_DISPLAYABLE_DIAGNOSTICS,
}
}

Expand All @@ -79,13 +82,8 @@ impl Execution {
&self.traversal_mode
}

pub(crate) fn get_max_diagnostics(&self) -> Option<u16> {
match self.traversal_mode {
TraversalMode::Check {
max_diagnostics, ..
} => Some(max_diagnostics),
_ => None,
}
pub(crate) fn get_max_diagnostics(&self) -> u16 {
self.max_diagnostics
}

/// `true` only when running the traversal in [TraversalMode::Check] and `should_fix` is `true`
Expand Down Expand Up @@ -128,7 +126,36 @@ impl Execution {

/// Based on the [mode](ExecutionMode), the function might launch a traversal of the file system
/// or handles the stdin file.
pub(crate) fn execute_mode(mode: Execution, mut session: CliSession) -> Result<(), Termination> {
pub(crate) fn execute_mode(
mut mode: Execution,
mut session: CliSession,
) -> Result<(), Termination> {
let max_diagnostics: Option<u16> = session
.args
.opt_value_from_str("--max-diagnostics")
.map_err(|source| Termination::ParseError {
argument: "--max-diagnostics",
source,
})?;

mode.max_diagnostics = if let Some(max_diagnostics) = max_diagnostics {
if max_diagnostics > MAXIMUM_DISPLAYABLE_DIAGNOSTICS {
return Err(Termination::OverflowNumberArgument(
"--max-diagnostics",
MAXIMUM_DISPLAYABLE_DIAGNOSTICS,
));
}

max_diagnostics
} else {
// The command `rome check` gives a default value of 20.
// In case of other commands that pass here, we limit to 50 to avoid to delay the terminal.
match &mode.traversal_mode {
TraversalMode::Check { .. } => 20,
TraversalMode::CI | TraversalMode::Format { .. } => 50,
}
};

// don't do any traversal if there's some content coming from stdin
if let Some((path, content)) = mode.as_stdin_file() {
let workspace = &*session.app.workspace;
Expand Down
85 changes: 52 additions & 33 deletions crates/rome_cli/src/traversal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ use rome_diagnostics::{
category, Advices, Category, DiagnosticExt, Error, FilePath, PrintDescription,
PrintDiagnostic, Severity, Visit,
},
MAXIMUM_DISPLAYABLE_DIAGNOSTICS,
};
use rome_fs::{AtomicInterner, FileSystem, OpenOptions, PathInterner, RomePath};
use rome_fs::{TraversalContext, TraversalScope};
Expand Down Expand Up @@ -83,13 +82,7 @@ pub(crate) fn traverse(execution: Execution, mut session: CliSession) -> Result<
let workspace = &*session.app.workspace;
let console = &mut *session.app.console;

// The command `rome check` gives a default value of 20.
// In case of other commands that pass here, we limit to 50 to avoid to delay the terminal.
// Once `--max-diagnostics` will be a global argument, `unwrap_of_default` should be enough.
let max_diagnostics = execution
.get_max_diagnostics()
.unwrap_or(MAXIMUM_DISPLAYABLE_DIAGNOSTICS);

let max_diagnostics = execution.get_max_diagnostics();
let remaining_diagnostics = AtomicU16::new(max_diagnostics);

let mut has_errors = false;
Expand Down Expand Up @@ -382,10 +375,23 @@ fn process_messages(options: ProcessMessagesOptions) {
}
}

let should_print = printed_diagnostics < max_diagnostics;
if should_print {
printed_diagnostics += 1;
remaining_diagnostics.store(
max_diagnostics.saturating_sub(printed_diagnostics),
Ordering::Relaxed,
);
} else {
not_printed_diagnostics += 1;
}

if mode.should_report_to_terminal() {
console.error(markup! {
{PrintDiagnostic(&err)}
});
if should_print {
console.error(markup! {
{PrintDiagnostic(&err)}
});
}
} else {
let file_name = err
.location()
Expand Down Expand Up @@ -478,31 +484,44 @@ fn process_messages(options: ProcessMessagesOptions) {
*has_errors = true;
}

let should_print = printed_diagnostics < max_diagnostics;
if should_print {
printed_diagnostics += 1;
remaining_diagnostics.store(
max_diagnostics.saturating_sub(printed_diagnostics),
Ordering::Relaxed,
);
} else {
not_printed_diagnostics += 1;
}

if mode.should_report_to_terminal() {
if mode.is_ci() {
let diag = CIDiffDiagnostic {
file_name: &file_name,
diff: FormatDiffAdvice {
old: &old,
new: &new,
},
};
if should_print {
if mode.is_ci() {
let diag = CIDiffDiagnostic {
file_name: &file_name,
diff: FormatDiffAdvice {
old: &old,
new: &new,
},
};

console.error(markup! {
{PrintDiagnostic(&diag)}
});
} else {
let diag = FormatDiffDiagnostic {
file_name: &file_name,
diff: FormatDiffAdvice {
old: &old,
new: &new,
},
};
console.error(markup! {
{PrintDiagnostic(&diag)}
});
} else {
let diag = FormatDiffDiagnostic {
file_name: &file_name,
diff: FormatDiffAdvice {
old: &old,
new: &new,
},
};

console.error(markup! {
{PrintDiagnostic(&diag)}
});
console.error(markup! {
{PrintDiagnostic(&diag)}
});
}
}
} else {
report.push_detail_report(ReportKind::Error(
Expand Down
53 changes: 53 additions & 0 deletions crates/rome_cli/tests/commands/check.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -777,3 +777,56 @@ fn files_max_size_parse_error() {
result,
));
}

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

for i in 0..60 {
let file_path = PathBuf::from(format!("src/file_{i}.js"));
fs.insert(file_path, LINT_ERROR.as_bytes());
}

let result = run_cli(
DynRef::Borrowed(&mut fs),
DynRef::Borrowed(&mut console),
Arguments::from_vec(vec![OsString::from("check"), OsString::from("src")]),
);

match result {
Err(Termination::CheckError) => {}
_ => panic!("run_cli returned {result:?} for a failed CI check, expected an error"),
}

assert_eq!(console.out_buffer.len(), 21);
}

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

for i in 0..60 {
let file_path = PathBuf::from(format!("src/file_{i}.js"));
fs.insert(file_path, LINT_ERROR.as_bytes());
}

let result = run_cli(
DynRef::Borrowed(&mut fs),
DynRef::Borrowed(&mut console),
Arguments::from_vec(vec![
OsString::from("check"),
OsString::from("--max-diagnostics"),
OsString::from("10"),
Path::new("src").as_os_str().into(),
]),
);

match result {
Err(Termination::CheckError) => {}
_ => panic!("run_cli returned {result:?} for a failed CI check, expected an error"),
}

assert_eq!(console.out_buffer.len(), 11);
}
53 changes: 53 additions & 0 deletions crates/rome_cli/tests/commands/ci.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -516,3 +516,56 @@ fn ci_runs_linter_not_formatter_issue_3495() {
result,
));
}

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

for i in 0..60 {
let file_path = PathBuf::from(format!("src/file_{i}.js"));
fs.insert(file_path, UNFORMATTED.as_bytes());
}

let result = run_cli(
DynRef::Borrowed(&mut fs),
DynRef::Borrowed(&mut console),
Arguments::from_vec(vec![OsString::from("ci"), OsString::from("src")]),
);

match result {
Err(Termination::CheckError) => {}
_ => panic!("run_cli returned {result:?} for a failed CI check, expected an error"),
}

assert_eq!(console.out_buffer.len(), 51);
}

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

for i in 0..60 {
let file_path = PathBuf::from(format!("src/file_{i}.js"));
fs.insert(file_path, UNFORMATTED.as_bytes());
}

let result = run_cli(
DynRef::Borrowed(&mut fs),
DynRef::Borrowed(&mut console),
Arguments::from_vec(vec![
OsString::from("ci"),
OsString::from("--max-diagnostics"),
OsString::from("10"),
OsString::from("src"),
]),
);

match result {
Err(Termination::CheckError) => {}
_ => panic!("run_cli returned {result:?} for a failed CI check, expected an error"),
}

assert_eq!(console.out_buffer.len(), 11);
}
Loading

0 comments on commit 50dbe68

Please sign in to comment.