From a7d1f7e1ec6d2d4443eaec4bee5faebd97f95c66 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Thu, 26 Oct 2023 11:08:13 +0530 Subject: [PATCH] Use `SourceKind::diff` for formatter (#8240) ## Summary This PR refactors the formatter diff code to reuse the `SourceKind::diff` logic. This has the benefit that the Notebook diff now includes the cell numbers which was not present before. ## Test Plan Update the snapshots and verified the cell numbers. --- crates/ruff_cli/src/commands/format.rs | 29 +++++++++++++++----- crates/ruff_cli/src/commands/format_stdin.rs | 13 ++------- crates/ruff_cli/tests/format.rs | 16 +++++++++-- crates/ruff_linter/src/source_kind.rs | 11 ++++++-- 4 files changed, 46 insertions(+), 23 deletions(-) diff --git a/crates/ruff_cli/src/commands/format.rs b/crates/ruff_cli/src/commands/format.rs index 54d135f296096..2e88d21b737f9 100644 --- a/crates/ruff_cli/src/commands/format.rs +++ b/crates/ruff_cli/src/commands/format.rs @@ -11,7 +11,6 @@ use itertools::Itertools; use log::{error, warn}; use rayon::iter::Either::{Left, Right}; use rayon::iter::{IntoParallelRefIterator, ParallelIterator}; -use similar::TextDiff; use thiserror::Error; use tracing::debug; @@ -472,11 +471,7 @@ impl<'a> FormatResults<'a> { }) .sorted_unstable_by_key(|(path, _, _)| *path) { - let text_diff = - TextDiff::from_lines(unformatted.source_code(), formatted.source_code()); - let mut unified_diff = text_diff.unified_diff(); - unified_diff.header(&fs::relativize_path(path), &fs::relativize_path(path)); - unified_diff.to_writer(&mut *f)?; + unformatted.diff(formatted, Some(path), f)?; } Ok(()) @@ -566,6 +561,7 @@ pub(crate) enum FormatCommandError { Read(Option, SourceError), Format(Option, FormatModuleError), Write(Option, SourceError), + Diff(Option, io::Error), } impl FormatCommandError { @@ -581,7 +577,8 @@ impl FormatCommandError { Self::Panic(path, _) | Self::Read(path, _) | Self::Format(path, _) - | Self::Write(path, _) => path.as_deref(), + | Self::Write(path, _) + | Self::Diff(path, _) => path.as_deref(), } } } @@ -649,6 +646,24 @@ impl Display for FormatCommandError { write!(f, "{}{} {err}", "Failed to format".bold(), ":".bold()) } } + Self::Diff(path, err) => { + if let Some(path) = path { + write!( + f, + "{}{}{} {err}", + "Failed to generate diff for ".bold(), + fs::relativize_path(path).bold(), + ":".bold() + ) + } else { + write!( + f, + "{}{} {err}", + "Failed to generate diff".bold(), + ":".bold() + ) + } + } Self::Panic(path, err) => { let message = r#"This indicates a bug in Ruff. If you could open an issue at: diff --git a/crates/ruff_cli/src/commands/format_stdin.rs b/crates/ruff_cli/src/commands/format_stdin.rs index 0e55551451359..082dfedfb00da 100644 --- a/crates/ruff_cli/src/commands/format_stdin.rs +++ b/crates/ruff_cli/src/commands/format_stdin.rs @@ -3,8 +3,6 @@ use std::path::Path; use anyhow::Result; use log::error; -use ruff_linter::fs; -use similar::TextDiff; use ruff_linter::source_kind::SourceKind; use ruff_python_ast::{PySourceType, SourceType}; @@ -109,14 +107,9 @@ fn format_source_code( } FormatMode::Check => {} FormatMode::Diff => { - let mut writer = stdout().lock(); - let text_diff = - TextDiff::from_lines(source_kind.source_code(), formatted.source_code()); - let mut unified_diff = text_diff.unified_diff(); - if let Some(path) = path { - unified_diff.header(&fs::relativize_path(path), &fs::relativize_path(path)); - } - unified_diff.to_writer(&mut writer).unwrap(); + source_kind + .diff(formatted, path, &mut stdout().lock()) + .map_err(|err| FormatCommandError::Diff(path.map(Path::to_path_buf), err))?; } }, FormattedSource::Unchanged => { diff --git a/crates/ruff_cli/tests/format.rs b/crates/ruff_cli/tests/format.rs index bfcb69ba0f679..bb1f547a24f58 100644 --- a/crates/ruff_cli/tests/format.rs +++ b/crates/ruff_cli/tests/format.rs @@ -509,21 +509,27 @@ fn test_diff() { success: false exit_code: 1 ----- stdout ----- - --- resources/test/fixtures/unformatted.ipynb - +++ resources/test/fixtures/unformatted.ipynb - @@ -1,12 +1,20 @@ + --- resources/test/fixtures/unformatted.ipynb:cell 1 + +++ resources/test/fixtures/unformatted.ipynb:cell 1 + @@ -1,3 +1,4 @@ import numpy -maths = (numpy.arange(100)**2).sum() -stats= numpy.asarray([1,2,3,4]).median() + +maths = (numpy.arange(100) ** 2).sum() +stats = numpy.asarray([1, 2, 3, 4]).median() + --- resources/test/fixtures/unformatted.ipynb:cell 3 + +++ resources/test/fixtures/unformatted.ipynb:cell 3 + @@ -1,4 +1,6 @@ # A cell with IPython escape command def some_function(foo, bar): pass + + %matplotlib inline + --- resources/test/fixtures/unformatted.ipynb:cell 4 + +++ resources/test/fixtures/unformatted.ipynb:cell 4 + @@ -1,5 +1,10 @@ foo = %pwd -def some_function(foo,bar,): + @@ -535,6 +541,7 @@ fn test_diff() { # Another cell with IPython escape command foo = %pwd print(foo) + --- resources/test/fixtures/unformatted.py +++ resources/test/fixtures/unformatted.py @@ -1,3 +1,3 @@ @@ -543,6 +550,7 @@ fn test_diff() { +y = 2 z = 3 + ----- stderr ----- 2 files would be reformatted, 1 file left unchanged "###); @@ -572,6 +580,7 @@ fn test_diff_no_change() { +y = 2 z = 3 + ----- stderr ----- 1 file would be reformatted "### @@ -605,6 +614,7 @@ fn test_diff_stdin_unformatted() { +y = 2 z = 3 + ----- stderr ----- "###); } diff --git a/crates/ruff_linter/src/source_kind.rs b/crates/ruff_linter/src/source_kind.rs index 1cca0017bbdaf..892eb5b595467 100644 --- a/crates/ruff_linter/src/source_kind.rs +++ b/crates/ruff_linter/src/source_kind.rs @@ -2,7 +2,7 @@ use std::io; use std::io::Write; use std::path::Path; -use anyhow::{bail, Result}; +use anyhow::Result; use similar::TextDiff; use thiserror::Error; @@ -88,7 +88,12 @@ impl SourceKind { } /// Write a diff of the transformed source file to `stdout`. - pub fn diff(&self, other: &Self, path: Option<&Path>, writer: &mut dyn Write) -> Result<()> { + pub fn diff( + &self, + other: &Self, + path: Option<&Path>, + writer: &mut dyn Write, + ) -> io::Result<()> { match (self, other) { (SourceKind::Python(src), SourceKind::Python(dst)) => { let text_diff = TextDiff::from_lines(src, dst); @@ -154,7 +159,7 @@ impl SourceKind { Ok(()) } - _ => bail!("cannot diff Python source code with Jupyter notebook source code"), + _ => panic!("cannot diff Python source code with Jupyter notebook source code"), } } }