Skip to content

Commit

Permalink
Use SourceKind::diff for formatter (#8240)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
dhruvmanila authored Oct 26, 2023
1 parent 88c8b47 commit a7d1f7e
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 23 deletions.
29 changes: 22 additions & 7 deletions crates/ruff_cli/src/commands/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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(())
Expand Down Expand Up @@ -566,6 +561,7 @@ pub(crate) enum FormatCommandError {
Read(Option<PathBuf>, SourceError),
Format(Option<PathBuf>, FormatModuleError),
Write(Option<PathBuf>, SourceError),
Diff(Option<PathBuf>, io::Error),
}

impl FormatCommandError {
Expand All @@ -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(),
}
}
}
Expand Down Expand Up @@ -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:
Expand Down
13 changes: 3 additions & 10 deletions crates/ruff_cli/src/commands/format_stdin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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 => {
Expand Down
16 changes: 13 additions & 3 deletions crates/ruff_cli/tests/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,):
+
Expand All @@ -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 @@
Expand All @@ -543,6 +550,7 @@ fn test_diff() {
+y = 2
z = 3
----- stderr -----
2 files would be reformatted, 1 file left unchanged
"###);
Expand Down Expand Up @@ -572,6 +580,7 @@ fn test_diff_no_change() {
+y = 2
z = 3
----- stderr -----
1 file would be reformatted
"###
Expand Down Expand Up @@ -605,6 +614,7 @@ fn test_diff_stdin_unformatted() {
+y = 2
z = 3
----- stderr -----
"###);
}
Expand Down
11 changes: 8 additions & 3 deletions crates/ruff_linter/src/source_kind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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"),
}
}
}
Expand Down

0 comments on commit a7d1f7e

Please sign in to comment.