Skip to content

Commit

Permalink
feat(cli): Dry run
Browse files Browse the repository at this point in the history
This change adds a new flag `--dry-run` that when
given, prevents otherwise destructive file
overwrite operations and instead prints a rich
diff. The diff contains names of files which would
be modified, and a git-like diff of what
modifications *would* be made.

The diff is human-readable, and not designed to be
easily machine-readable.

The change leverages the `similar` crate, on which
we transitively depended on already anyway.

Closes #152.
  • Loading branch information
alexpovel committed Oct 27, 2024
1 parent 9b9010f commit 4d8a000
Show file tree
Hide file tree
Showing 8 changed files with 137 additions and 11 deletions.
5 changes: 3 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ ignore = "0.4.23"
itertools = "0.13.0"
log = "0.4.22"
pathdiff = "0.2.1"
similar = "2.6.0"
tempfile = "3.13.0"
titlecase = "3.3.0"
tree-sitter = "0.23.0"
Expand Down
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1457,6 +1457,12 @@ Options (global):
Processing no files is not an error condition in itself, but might be an
unexpected outcome in some contexts. This flag makes the condition explicit.

--dry-run
Do not destructively overwrite files, instead print rich diff only.
The rich diff contains file names and changes which would be performed
outside of dry running.

-i, --invert
Undo the effects of passed actions, where applicable.
Expand Down
44 changes: 38 additions & 6 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use ignore::{WalkBuilder, WalkState};
use itertools::Itertools;
use log::{debug, error, info, trace, LevelFilter};
use pathdiff::diff_paths;
use similar::{ChangeTag, TextDiff};
#[cfg(feature = "german")]
use srgn::actions::German;
use srgn::actions::{
Expand Down Expand Up @@ -561,7 +562,7 @@ fn process_path(

debug!("Processing path: {:?}", path);

let (new_contents, filesize, changed) = {
let (new_contents, old_contents, filesize, changed) = {
let mut file = File::open(&path)?;

let filesize = file.metadata().map_or(0, |m| m.len());
Expand All @@ -580,7 +581,7 @@ fn process_path(
actions,
)?;

(destination, filesize, changed)
(destination, source, filesize, changed)
};

// Hold the lock so results aren't intertwined
Expand Down Expand Up @@ -610,11 +611,36 @@ fn process_path(
}

if changed {
debug!("Got new file contents, writing to file: {:?}", path);
fs::write(&path, new_contents.as_bytes())?;
trace!(
"File contents changed, will process file: {}",
path.display()
);

if global_options.dry_run {
debug!(
"Dry-running: will not overwrite file '{}' with new contents: {}",
path.display(),
new_contents.escape_debug()
);
writeln!(stdout, "{}", format!("{}:", path.display()).magenta())?;

let diff = TextDiff::from_lines(&old_contents, &new_contents);
for change in diff.iter_all_changes() {
let (sign, color) = match change.tag() {
ChangeTag::Delete => ('-', Color::Red),
ChangeTag::Insert => ('+', Color::Green),
ChangeTag::Equal => continue,
};

// Confirm after successful write.
writeln!(stdout, "{}", path.display())?;
write!(stdout, "{}", format!("{sign} {change}").color(color))?;
}
} else {
debug!("Got new file contents, writing to file: {:?}", path);
fs::write(&path, new_contents.as_bytes())?;

// Confirm after successful processing.
writeln!(stdout, "{}", path.display())?;
}
} else {
debug!(
"Skipping writing file anew (nothing changed): {}",
Expand Down Expand Up @@ -1047,6 +1073,12 @@ mod cli {
/// unexpected outcome in some contexts. This flag makes the condition explicit.
#[arg(long, verbatim_doc_comment, alias = "fail-empty-glob")]
pub fail_no_files: bool,
/// Do not destructively overwrite files, instead print rich diff only.
///
/// The rich diff contains file names and changes which would be performed
/// outside of dry running.
#[arg(long, verbatim_doc_comment)]
pub dry_run: bool,
/// Undo the effects of passed actions, where applicable.
///
/// Requires a 1:1 mapping between replacements and original, which is currently
Expand Down
16 changes: 13 additions & 3 deletions tests/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ Heizoelrueckstossabdaempfung.
#[case] input: PathBuf,
#[case] args: &[&str],
#[case] skip_output_check: bool,
#[values(true, false)] dry_run: bool,
) -> anyhow::Result<()> {
use std::mem::ManuallyDrop;

Expand All @@ -409,7 +410,11 @@ Heizoelrueckstossabdaempfung.
// Arrange
let mut cmd = get_cmd();

let baseline = {
let baseline = if dry_run {
// Stays the same! In dry runs, we compare against the very same directory,
// as it should not change.
input.clone()
} else {
let mut baseline = input.clone();
baseline.pop();
baseline.push("out");
Expand All @@ -426,6 +431,9 @@ Heizoelrueckstossabdaempfung.
["--stdin-override-to", "false"],
);
cmd.args(&args);
if dry_run {
cmd.arg("--dry-run");
}

// Act
let output = cmd.output().expect("failed to execute binary under test");
Expand All @@ -435,15 +443,17 @@ Heizoelrueckstossabdaempfung.
// Thing itself works
assert!(output.status.success(), "Binary execution itself failed");

// Results are correct Do not drop on panic, to keep tmpdir in place for manual
// inspection. Can then diff directories.
// Do not drop on panic, to keep tmpdir in place for manual inspection. Can then
// diff directories.
check_directories_equality(baseline, candidate.path().to_owned())?;

// Test was successful: ok to drop.
drop(ManuallyDrop::into_inner(candidate));

// Let's look at command output now.
if !skip_output_check {
snapshot_name.push_str(if dry_run { "-dry-run" } else { "" });

// These are inherently platform-specific, as they deal with file paths.
snapshot_name.push('-');
snapshot_name.push_str(std::env::consts::OS);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
source: tests/cli.rs
expression: "CommandSnap\n{\n args, stdin: None, stdout:\n stdout.split_inclusive('\\n').map(ToOwned::to_owned).collect_vec(),\n exit_code,\n}"
info:
stderr: []
---
args:
- "--sorted"
- "--glob"
- "**/*.py"
- foo
- baz
stdin: ~
stdout:
- "1.py:\n"
- "- # This string is found and touched: foo\n"
- "+ # This string is found and touched: baz\n"
- "- def foo(bar: int) -> int:\n"
- "+ def baz(bar: int) -> int:\n"
- "subdir/2.py:\n"
- "- def foo(bar: int) -> int:\n"
- "+ def baz(bar: int) -> int:\n"
- "subdir/subdir/3.py:\n"
- "- def foo(bar: int) -> int:\n"
- "+ def baz(bar: int) -> int:\n"
exit_code: 0
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
---
source: tests/cli.rs
expression: "CommandSnap\n{\n args, stdin: None, stdout:\n stdout.split_inclusive('\\n').map(ToOwned::to_owned).collect_vec(),\n exit_code,\n}"
info:
stderr: []
---
args:
- "--sorted"
- "--python"
- function-names
- "--glob"
- subdir/**/*.py
- foo
- baz
stdin: ~
stdout:
- "subdir/2.py:\n"
- "- def foo(bar: int) -> int:\n"
- "+ def baz(bar: int) -> int:\n"
- "subdir/subdir/3.py:\n"
- "- def foo(bar: int) -> int:\n"
- "+ def baz(bar: int) -> int:\n"
exit_code: 0
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
---
source: tests/cli.rs
expression: "CommandSnap\n{\n args, stdin: None, stdout:\n stdout.split_inclusive('\\n').map(ToOwned::to_owned).collect_vec(),\n exit_code,\n}"
info:
stderr: []
---
args:
- "--sorted"
- "--python"
- function-names
- foo
- baz
stdin: ~
stdout:
- "1-shebanged:\n"
- "- def foo(bar: int) -> int:\n"
- "+ def baz(bar: int) -> int:\n"
- "1.py:\n"
- "- def foo(bar: int) -> int:\n"
- "+ def baz(bar: int) -> int:\n"
- "subdir/2.py:\n"
- "- def foo(bar: int) -> int:\n"
- "+ def baz(bar: int) -> int:\n"
- "subdir/subdir/3.py:\n"
- "- def foo(bar: int) -> int:\n"
- "+ def baz(bar: int) -> int:\n"
exit_code: 0

0 comments on commit 4d8a000

Please sign in to comment.