From 2e1bca2a46c2cf1cddee6eb3ac65d936cb5258cd Mon Sep 17 00:00:00 2001 From: l3ops Date: Tue, 8 Nov 2022 17:48:19 +0100 Subject: [PATCH 1/2] fix(rome_cli): correctly account for diff diagnostics in the printed diagnostics count --- crates/rome_cli/src/commands/check.rs | 28 +-------- crates/rome_cli/src/commands/help.rs | 6 +- crates/rome_cli/src/execute.rs | 49 +++++++++++---- crates/rome_cli/src/traversal.rs | 85 ++++++++++++++++----------- 4 files changed, 95 insertions(+), 73 deletions(-) diff --git a/crates/rome_cli/src/commands/check.rs b/crates/rome_cli/src/commands/check.rs index 7dd60c9d8c9..ad8392c185a 100644 --- a/crates/rome_cli/src/commands/check.rs +++ b/crates/rome_cli/src/commands/check.rs @@ -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 = 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 @@ -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, ) } diff --git a/crates/rome_cli/src/commands/help.rs b/crates/rome_cli/src/commands/help.rs index 70b4f492895..70817f7da60 100644 --- a/crates/rome_cli/src/commands/help.rs +++ b/crates/rome_cli/src/commands/help.rs @@ -61,7 +61,8 @@ const CI: Markup = markup! { ""OPTIONS:"" ""--formatter-enabled"" Allow to enable or disable the formatter check. (default: true) - ""--linter-enabled"" Allow to enable or disable the linter check. (default: true)" + ""--linter-enabled"" Allow to enable or disable the linter check. (default: true) + ""--max-diagnostics"" Cap the amount of diagnostics displayed (default: 50)" {FORMAT_OPTIONS} }; @@ -75,7 +76,8 @@ const FORMAT: Markup = markup! { ""OPTIONS:"" ""--write"" Edit the files in place (beware!) instead of printing the diff to the console - ""--skip-errors"" Skip over files containing syntax errors instead of emitting an error diagnostic." + ""--skip-errors"" Skip over files containing syntax errors instead of emitting an error diagnostic. + ""--max-diagnostics"" Cap the amount of diagnostics displayed (default: 50)" {FORMAT_OPTIONS} """--stdin-file-path "" 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 " diff --git a/crates/rome_cli/src/execute.rs b/crates/rome_cli/src/execute.rs index e557b11ea8c..7937557dfcf 100644 --- a/crates/rome_cli/src/execute.rs +++ b/crates/rome_cli/src/execute.rs @@ -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, @@ -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` @@ -59,6 +60,7 @@ impl Execution { Self { report_mode: ReportMode::default(), traversal_mode: mode, + max_diagnostics: MAXIMUM_DISPLAYABLE_DIAGNOSTICS, } } @@ -67,6 +69,7 @@ impl Execution { Self { traversal_mode, report_mode, + max_diagnostics: MAXIMUM_DISPLAYABLE_DIAGNOSTICS, } } @@ -79,13 +82,8 @@ impl Execution { &self.traversal_mode } - pub(crate) fn get_max_diagnostics(&self) -> Option { - 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` @@ -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 = 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; diff --git a/crates/rome_cli/src/traversal.rs b/crates/rome_cli/src/traversal.rs index 3ae79e915c1..822c4a4514b 100644 --- a/crates/rome_cli/src/traversal.rs +++ b/crates/rome_cli/src/traversal.rs @@ -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}; @@ -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; @@ -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() @@ -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( From 6e823001c86c10e27f2687e21115fa1d74a90cdc Mon Sep 17 00:00:00 2001 From: l3ops Date: Tue, 8 Nov 2022 17:57:00 +0100 Subject: [PATCH 2/2] add tests --- crates/rome_cli/tests/commands/check.rs | 53 ++++++++++++++++++++++++ crates/rome_cli/tests/commands/ci.rs | 53 ++++++++++++++++++++++++ crates/rome_cli/tests/commands/format.rs | 47 +++++++++++++++++++++ 3 files changed, 153 insertions(+) diff --git a/crates/rome_cli/tests/commands/check.rs b/crates/rome_cli/tests/commands/check.rs index f8f504fe624..03ed983ae64 100644 --- a/crates/rome_cli/tests/commands/check.rs +++ b/crates/rome_cli/tests/commands/check.rs @@ -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); +} diff --git a/crates/rome_cli/tests/commands/ci.rs b/crates/rome_cli/tests/commands/ci.rs index 95c3c89dfa5..67eb66aa9b3 100644 --- a/crates/rome_cli/tests/commands/ci.rs +++ b/crates/rome_cli/tests/commands/ci.rs @@ -511,3 +511,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); +} diff --git a/crates/rome_cli/tests/commands/format.rs b/crates/rome_cli/tests/commands/format.rs index a5fc0359b3b..16fe0ffc88c 100644 --- a/crates/rome_cli/tests/commands/format.rs +++ b/crates/rome_cli/tests/commands/format.rs @@ -1153,3 +1153,50 @@ 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, UNFORMATTED.as_bytes()); + } + + let result = run_cli( + DynRef::Borrowed(&mut fs), + DynRef::Borrowed(&mut console), + Arguments::from_vec(vec![OsString::from("format"), OsString::from("src")]), + ); + + assert!(result.is_ok(), "run_cli returned {result:?}"); + + assert_eq!(console.out_buffer.len(), 52); +} + +#[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("format"), + OsString::from("--max-diagnostics"), + OsString::from("10"), + OsString::from("src"), + ]), + ); + + assert!(result.is_ok(), "run_cli returned {result:?}"); + + assert_eq!(console.out_buffer.len(), 12); +}