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

feat(rome_cli): integrate the new diff printing with CI mode #2337

Merged
merged 1 commit into from
Apr 1, 2022
Merged

feat(rome_cli): integrate the new diff printing with CI mode #2337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions .cargo/config.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 --"
98 changes: 79 additions & 19 deletions crates/rome_cli/src/commands/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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>"error[CI]"</Error>": File content differs from formatting output\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think we create a console.br() function that adds a line break? Rome classic had something like this: reporter.br().

<Info>"[Diff not printed for file over 1Mb]\n"</Info>
});
continue;
}

let diff = Diff {
mode: DiffMode::Unified,
left: &old,
right: &new,
};

session.app.console.message(markup! {
{file_name}": "
<Error>"error[CI]"</Error>": File content differs from formatting output\n"
{diff}
});
}
}
}

// Formatting emitted error diagnostics, exit with a non-zero code
Expand Down Expand Up @@ -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<Diagnostic>,
diagnostics: Sender<Error>,
}

impl<'ctx, 'app> FormatCommandOptions<'ctx, 'app> {
Expand All @@ -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();
}
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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());
}
}
}
Expand All @@ -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());
}
}
}
Expand All @@ -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<Vec<Diagnostic>, Diagnostic> {
fn format_file(params: FormatFileParams) -> Result<Vec<Diagnostic>, 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());
Expand Down Expand Up @@ -354,11 +398,11 @@ fn format_file(params: FormatFileParams) -> Result<Vec<Diagnostic>, 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)?;
Expand All @@ -367,6 +411,22 @@ fn format_file(params: FormatFileParams) -> Result<Vec<Diagnostic>, 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<Diagnostic> for Error {
fn from(diagnostic: Diagnostic) -> Self {
Self::Diagnostic(diagnostic)
}
}

/// Extension trait for turning [Display]-able error types into [Diagnostic]
trait ResultExt {
type Result;
Expand Down
36 changes: 25 additions & 11 deletions crates/rome_console/src/diff.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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! {
<Error>"- "{change.value()}</Error>
<Error>"- "{line}</Error>
})?;
}
ChangeTag::Insert => {
fmt.write_markup(markup! {
<Success>"+ "{change.value()}</Success>
<Success>"+ "{line}</Success>
})?;
}
ChangeTag::Equal => {
fmt.write_str(" ")?;
fmt.write_str(change.value())?;
fmt.write_str(line)?;
}
}

if change.missing_newline() {
writeln!(fmt)?;
}
writeln!(fmt)?;
}
}
DiffMode::Split => {
Expand All @@ -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());
Expand All @@ -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));
}
}
}
Expand Down