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

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

Merged
merged 2 commits into from
Nov 9, 2022
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
28 changes: 1 addition & 27 deletions crates/rome_cli/src/commands/check.rs
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
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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Loading