From a5f6e17c456b5ad25073b263e8278ba4c089e12d Mon Sep 17 00:00:00 2001 From: l3ops Date: Thu, 31 Mar 2022 17:59:26 +0200 Subject: [PATCH] feat(rome_cli): integrate the new diff printing with CI mode --- .cargo/config.toml | 1 + crates/rome_cli/src/commands/format.rs | 98 +++++++++++++++++++++----- crates/rome_console/src/diff.rs | 36 +++++++--- 3 files changed, 105 insertions(+), 30 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index d29df3626dc..55994854bbe 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -5,3 +5,4 @@ codegen = "run -p xtask_codegen --" bench_parser = "run -p xtask_bench --release -- --feature parser" bench_formatter = "run -p xtask_bench --release -- --feature formatter" coverage = "run -p xtask_coverage --release --" +rome-cli = "run -p rome_cli --release --" diff --git a/crates/rome_cli/src/commands/format.rs b/crates/rome_cli/src/commands/format.rs index fb749c2791e..03e72f01daa 100644 --- a/crates/rome_cli/src/commands/format.rs +++ b/crates/rome_cli/src/commands/format.rs @@ -10,6 +10,10 @@ use std::{ }; use crossbeam::channel::{unbounded, Sender}; +use rome_console::{ + diff::{Diff, DiffMode}, + markup, +}; use rome_core::App; use rome_diagnostics::{ file::{FileId, Files, SimpleFile}, @@ -126,10 +130,18 @@ pub(crate) fn format(mut session: CliSession) -> Result<(), Termination> { let mut file_ids = HashSet::new(); let mut diagnostics = Vec::new(); - while let Ok(diag) = recv_diags.recv() { - has_errors |= diag.is_error(); - file_ids.insert(diag.file_id); - diagnostics.push(diag); + while let Ok(error) = recv_diags.recv() { + match &error { + Error::Diagnostic(diag) => { + has_errors |= diag.is_error(); + file_ids.insert(diag.file_id); + } + Error::Diff { .. } => { + has_errors = true; + } + } + + diagnostics.push(error); } let mut files = PathFiles::default(); @@ -170,8 +182,40 @@ pub(crate) fn format(mut session: CliSession) -> Result<(), Termination> { files.storage.insert(file_id, SimpleFile::new(name, source)); } - for diag in diagnostics { - session.app.console.diagnostic(&files, &diag); + for error in diagnostics { + match error { + Error::Diagnostic(diag) => { + session.app.console.diagnostic(&files, &diag); + } + Error::Diff { + file_name, + old, + new, + } => { + // Skip printing the diff for files over 1Mb (probably a minified file) + let max_len = old.len().max(new.len()); + if max_len >= 1_000_000 { + session.app.console.message(markup! { + {file_name}": " + "error[CI]"": File content differs from formatting output\n" + "[Diff not printed for file over 1Mb]\n" + }); + continue; + } + + let diff = Diff { + mode: DiffMode::Unified, + left: &old, + right: &new, + }; + + session.app.console.message(markup! { + {file_name}": " + "error[CI]"": File content differs from formatting output\n" + {diff} + }); + } + } } // Formatting emitted error diagnostics, exit with a non-zero code @@ -225,7 +269,7 @@ struct FormatCommandOptions<'ctx, 'app> { /// Shared atomic counter storing the number of formatted files formatted: &'ctx AtomicUsize, /// Channel sending diagnostics to the display thread - diagnostics: Sender, + diagnostics: Sender, } impl<'ctx, 'app> FormatCommandOptions<'ctx, 'app> { @@ -235,7 +279,7 @@ impl<'ctx, 'app> FormatCommandOptions<'ctx, 'app> { } /// Send a diagnostic to the display thread - fn push_diagnostic(&self, err: Diagnostic) { + fn push_diagnostic(&self, err: Error) { self.diagnostics.send(err).ok(); } } @@ -246,7 +290,7 @@ impl<'ctx, 'app> TraversalContext for FormatCommandOptions<'ctx, 'app> { } fn push_diagnostic(&self, file_id: FileId, code: &'static str, title: String) { - self.push_diagnostic(Diagnostic::error(file_id, code, title)); + self.push_diagnostic(Diagnostic::error(file_id, code, title).into()); } fn can_handle(&self, rome_path: &RomePath) -> bool { @@ -277,7 +321,7 @@ fn handle_file(ctx: &FormatCommandOptions, path: &Path, file_id: FileId) { ctx.add_formatted(); } else { for error in errors { - ctx.push_diagnostic(error); + ctx.push_diagnostic(error.into()); } } } @@ -293,7 +337,7 @@ fn handle_file(ctx: &FormatCommandOptions, path: &Path, file_id: FileId) { }, }; - ctx.push_diagnostic(Diagnostic::error(file_id, "Panic", msg)); + ctx.push_diagnostic(Diagnostic::error(file_id, "Panic", msg).into()); } } } @@ -311,13 +355,13 @@ struct FormatFileParams<'ctx, 'app> { /// (or map it into memory it), parse and format it; then, it either writes the /// result back or compares it with the original content and emit a diagnostic #[tracing::instrument(level = "trace", skip_all, fields(path = ?params.path))] -fn format_file(params: FormatFileParams) -> Result, Diagnostic> { +fn format_file(params: FormatFileParams) -> Result, Error> { if !params.app.can_format(&RomePath::new(params.path)) { - return Err(Diagnostic::error( + return Err(Error::from(Diagnostic::error( params.file_id, "IO", "unhandled file type", - )); + ))); } let source_type = SourceType::try_from(params.path).unwrap_or_else(|_| SourceType::js_module()); @@ -354,11 +398,11 @@ fn format_file(params: FormatFileParams) -> Result, Diagnostic> if params.is_check { let has_diff = output != input.as_bytes(); if has_diff { - return Err(Diagnostic::error( - params.file_id, - "CI", - "File content differs from formatting output", - )); + return Err(Error::Diff { + file_name: params.path.display().to_string(), + old: input, + new: result.into_code(), + }); } } else { file.set_content(output).with_file_id(params.file_id)?; @@ -367,6 +411,22 @@ fn format_file(params: FormatFileParams) -> Result, Diagnostic> Ok(Vec::new()) } +/// Wrapper type for errors that may happen during the formatting process +enum Error { + Diagnostic(Diagnostic), + Diff { + file_name: String, + old: String, + new: String, + }, +} + +impl From for Error { + fn from(diagnostic: Diagnostic) -> Self { + Self::Diagnostic(diagnostic) + } +} + /// Extension trait for turning [Display]-able error types into [Diagnostic] trait ResultExt { type Result; diff --git a/crates/rome_console/src/diff.rs b/crates/rome_console/src/diff.rs index a1d529e95be..1efa29a0be4 100644 --- a/crates/rome_console/src/diff.rs +++ b/crates/rome_console/src/diff.rs @@ -77,6 +77,12 @@ pub struct Diff<'a> { pub right: &'a str, } +/// Lines longer than this will have the remaining characters clipped +/// +/// This is intended as a safety check to avoid overflowing the console, +/// for instance when printing minified files +const MAX_LINE_LENGTH: usize = 250; + impl<'a> Display for Diff<'a> { fn fmt(&self, fmt: &mut Formatter) -> io::Result<()> { let diff = TextDiff::from_lines(self.left, self.right); @@ -96,7 +102,9 @@ impl<'a> Display for Diff<'a> { } for change in hunk.iter_changes() { - max_line_length = max_line_length.max(change.value().trim_end().len()); + let line = change.value().trim_end(); + let line_length = line.len().min(MAX_LINE_LENGTH); + max_line_length = max_line_length.max(line_length); } } @@ -143,26 +151,28 @@ impl<'a> Display for Diff<'a> { fmt.write_str(" | ")?; + let line = change.value().trim_end(); + let line_length = line.len().min(MAX_LINE_LENGTH); + let line = &line[..line_length]; + match change.tag() { ChangeTag::Delete => { fmt.write_markup(markup! { - "- "{change.value()} + "- "{line} })?; } ChangeTag::Insert => { fmt.write_markup(markup! { - "+ "{change.value()} + "+ "{line} })?; } ChangeTag::Equal => { fmt.write_str(" ")?; - fmt.write_str(change.value())?; + fmt.write_str(line)?; } } - if change.missing_newline() { - writeln!(fmt)?; - } + writeln!(fmt)?; } } DiffMode::Split => { @@ -176,12 +186,16 @@ impl<'a> Display for Diff<'a> { let mut right = Vec::new(); for change in hunk.iter_changes() { + let line = change.value().trim_end(); + let line_length = line.len().min(MAX_LINE_LENGTH); + let line = &line[..line_length]; + match change.tag() { ChangeTag::Delete => { - left.push(Some(change.value().trim_end())); + left.push(Some(line)); } ChangeTag::Insert => { - right.push(Some(change.value().trim_end())); + right.push(Some(line)); } ChangeTag::Equal => { let position = left.len().max(right.len()); @@ -195,8 +209,8 @@ impl<'a> Display for Diff<'a> { right.resize(position, None); } - left.push(Some(change.value().trim_end())); - right.push(Some(change.value().trim_end())); + left.push(Some(line)); + right.push(Some(line)); } } }