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

fix(rome_cli): exit with error code while applying fixes #4270

Merged
merged 4 commits into from
Mar 10, 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
87 changes: 82 additions & 5 deletions crates/rome_cli/src/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use rome_console::fmt::Formatter;
use rome_console::fmt::{Display, Formatter};
use rome_console::markup;
use rome_diagnostics::adapters::{IoError, PicoArgsError};
use rome_diagnostics::{
Expand Down Expand Up @@ -36,6 +36,10 @@ pub enum CliDiagnostic {
IncompatibleArguments(IncompatibleArguments),
/// Returned by a traversal command when error diagnostics were emitted
CheckError(CheckError),
/// Emitted when a file is fixed, but it still contains diagnostics.
///
/// This happens when these diagnostics come from rules that don't have a code action.
FileCheckApply(FileCheckApply),
/// When an argument is higher than the expected maximum
OverflowNumberArgument(OverflowNumberArgument),
/// Wrapper for an underlying `rome_service` error
Expand Down Expand Up @@ -147,9 +151,57 @@ pub struct IncompatibleArguments {
#[diagnostic(
category = "internalError/io",
severity = Error,
message = "Some errors were emitted while running checks"
message(
description = "{action_kind}",
message({{self.action_kind}})
)
)]
pub struct CheckError {
action_kind: CheckActionKind,
}

#[derive(Debug, Clone, Copy)]
pub enum CheckActionKind {
Check,
Apply,
}

impl Display for CheckActionKind {
fn fmt(&self, fmt: &mut Formatter) -> std::io::Result<()> {
match self {
CheckActionKind::Check => fmt.write_markup(markup! {
"Some errors were emitted while "<Emphasis>"running checks"</Emphasis>
}),
CheckActionKind::Apply => fmt.write_markup(markup! {
"Some errors were emitted while "<Emphasis>"applying fixes"</Emphasis>
}),
}
}
}

impl std::fmt::Display for CheckActionKind {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
CheckActionKind::Check => {
write!(f, "Some errors were emitted while running checks")
}
CheckActionKind::Apply => {
write!(f, "Some errors were emitted while applying fixes")
}
}
}
}

#[derive(Debug, Diagnostic)]
#[diagnostic(
category = "internalError/io",
severity = Error,
message = "Fixes applied to the file, but there a still diagnostics to address."
)]
pub struct CheckError;
pub struct FileCheckApply {
#[location(resource)]
pub file_path: String,
}

#[derive(Debug, Diagnostic)]
#[diagnostic(
Expand Down Expand Up @@ -300,9 +352,25 @@ impl CliDiagnostic {
})
}

/// Emitted when errors were emitted while traversing the file system
/// Emitted when errors were emitted while running `check` command
pub fn check_error() -> Self {
Self::CheckError(CheckError)
Self::CheckError(CheckError {
action_kind: CheckActionKind::Check,
})
}

/// Emitted when errors were emitted while apply code fixes
pub fn apply_error() -> Self {
Self::CheckError(CheckError {
action_kind: CheckActionKind::Apply,
})
}

/// Emitted for a file that has code fixes, but still has diagnostics to address
pub fn file_apply_error(file_path: impl Into<String>) -> Self {
Self::FileCheckApply(FileCheckApply {
file_path: file_path.into(),
})
}

/// Emitted when the server is not running
Expand Down Expand Up @@ -353,6 +421,7 @@ impl Diagnostic for CliDiagnostic {
CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.category(),
CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.category(),
CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.category(),
CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.category(),
}
}

Expand All @@ -372,6 +441,7 @@ impl Diagnostic for CliDiagnostic {
CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.tags(),
CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.tags(),
CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.tags(),
CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.tags(),
}
}

Expand All @@ -391,6 +461,7 @@ impl Diagnostic for CliDiagnostic {
CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.severity(),
CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.severity(),
CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.severity(),
CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.severity(),
}
}

Expand All @@ -410,6 +481,7 @@ impl Diagnostic for CliDiagnostic {
CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.location(),
CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.location(),
CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.location(),
CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.location(),
}
}

Expand All @@ -429,6 +501,7 @@ impl Diagnostic for CliDiagnostic {
CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.message(fmt),
CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.message(fmt),
CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.message(fmt),
CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.message(fmt),
}
}

Expand All @@ -448,6 +521,7 @@ impl Diagnostic for CliDiagnostic {
CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.description(fmt),
CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.description(fmt),
CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.description(fmt),
CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.description(fmt),
}
}

Expand All @@ -467,6 +541,7 @@ impl Diagnostic for CliDiagnostic {
CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.advices(visitor),
CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.advices(visitor),
CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.advices(visitor),
CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.advices(visitor),
}
}

Expand All @@ -490,6 +565,7 @@ impl Diagnostic for CliDiagnostic {
diagnostic.verbose_advices(visitor)
}
CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.verbose_advices(visitor),
CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.verbose_advices(visitor),
}
}

Expand All @@ -509,6 +585,7 @@ impl Diagnostic for CliDiagnostic {
CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.source(),
CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.source(),
CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.source(),
CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.source(),
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions crates/rome_cli/src/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,13 @@ impl Execution {
matches!(self.traversal_mode, TraversalMode::Check { .. })
}

pub(crate) const fn is_check_apply(&self) -> bool {
match self.traversal_mode {
TraversalMode::Check { fix_file_mode } => fix_file_mode.is_some(),
_ => false,
}
}

pub(crate) const fn is_format(&self) -> bool {
matches!(self.traversal_mode, TraversalMode::Format { .. })
}
Expand Down
34 changes: 32 additions & 2 deletions crates/rome_cli/src/traversal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,11 @@ pub(crate) fn traverse(execution: Execution, mut session: CliSession) -> Result<
if count.saturating_sub(skipped) == 0 {
Err(CliDiagnostic::no_files_processed())
} else if errors > 0 {
Err(CliDiagnostic::check_error())
if execution.is_check_apply() {
Err(CliDiagnostic::apply_error())
} else {
Err(CliDiagnostic::check_error())
}
} else {
Ok(())
}
Expand Down Expand Up @@ -371,6 +375,25 @@ fn process_messages(options: ProcessMessagesOptions) {
total_skipped_suggested_fixes += skipped_suggested_fixes;
}

Message::ApplyError(error) => {
*errors += 1;
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() && should_print {
console.error(markup! {
{if verbose { PrintDiagnostic::verbose(&error) } else { PrintDiagnostic::simple(&error) }}
});
}
}

Message::Error(mut err) => {
let location = err.location();
if let Some(Resource::File(file_path)) = location.resource.as_ref() {
Expand Down Expand Up @@ -825,7 +848,13 @@ fn process_file(ctx: &TraversalOptions, path: &Path) -> FileResult {
.with_file_path(path.display().to_string())?;
}

return Ok(FileStatus::Success);
return if fixed.errors == 0 {
Ok(FileStatus::Success)
} else {
Ok(FileStatus::Message(Message::ApplyError(
CliDiagnostic::file_apply_error(path.display().to_string()),
)))
};
}

let categories = if ctx.execution.is_format() || supported_lint.reason.is_some() {
Expand Down Expand Up @@ -942,6 +971,7 @@ enum Message {
/// Suggested fixes skipped during the lint traversal
skipped_suggested_fixes: u32,
},
ApplyError(CliDiagnostic),
Error(Error),
Diagnostics {
name: String,
Expand Down
Loading