-
Notifications
You must be signed in to change notification settings - Fork 892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
check_diff
function rewrite
#6390
base: master
Are you sure you want to change the base?
Changes from all commits
dc5d3a1
613dacb
1e048de
2670a61
aba3ce4
90e1264
3c46b92
e7ee88d
b972394
4578196
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,10 +1,14 @@ | ||||||
use diffy; | ||||||
use std::env; | ||||||
use std::io; | ||||||
use std::fmt::Debug; | ||||||
use std::io::{self, Write}; | ||||||
use std::path::{Path, PathBuf}; | ||||||
use std::process::Command; | ||||||
use std::process::{Command, Stdio}; | ||||||
use std::str::Utf8Error; | ||||||
use tracing::info; | ||||||
use walkdir::WalkDir; | ||||||
|
||||||
#[derive(Debug)] | ||||||
pub enum CheckDiffError { | ||||||
/// Git related errors | ||||||
FailedGit(GitError), | ||||||
|
@@ -18,6 +22,9 @@ pub enum CheckDiffError { | |||||
FailedBinaryVersioning(PathBuf), | ||||||
/// Error when obtaining cargo version | ||||||
FailedCargoVersion(&'static str), | ||||||
/// Error when trying to find rust files | ||||||
FailedFindingRustFiles(&'static str), | ||||||
FailedWritingToFile(&'static str), | ||||||
IO(std::io::Error), | ||||||
} | ||||||
|
||||||
|
@@ -39,6 +46,7 @@ impl From<Utf8Error> for CheckDiffError { | |||||
} | ||||||
} | ||||||
|
||||||
#[derive(Debug)] | ||||||
pub enum GitError { | ||||||
FailedClone { stdout: Vec<u8>, stderr: Vec<u8> }, | ||||||
FailedRemoteAdd { stdout: Vec<u8>, stderr: Vec<u8> }, | ||||||
|
@@ -54,17 +62,35 @@ impl From<io::Error> for GitError { | |||||
} | ||||||
|
||||||
// will be used in future PRs, just added to make the compiler happy | ||||||
#[allow(dead_code)] | ||||||
pub struct CheckDiffRunners { | ||||||
feature_runner: RustfmtRunner, | ||||||
src_runner: RustfmtRunner, | ||||||
pub feature_runner: RustfmtRunner, | ||||||
pub src_runner: RustfmtRunner, | ||||||
} | ||||||
|
||||||
pub struct RustfmtRunner { | ||||||
ld_library_path: String, | ||||||
binary_path: PathBuf, | ||||||
} | ||||||
|
||||||
impl CheckDiffRunners { | ||||||
/// Creates a diff generated by running the source and feature binaries on the same file path | ||||||
pub fn create_diff( | ||||||
&self, | ||||||
path: &Path, | ||||||
additional_configs: &Option<Vec<String>>, | ||||||
) -> Result<String, CheckDiffError> { | ||||||
let code = std::fs::read_to_string(path)?; | ||||||
let src_format = self.src_runner.format_code(&code, additional_configs)?; | ||||||
let feature_format = self.feature_runner.format_code(&code, additional_configs)?; | ||||||
let diff = diffy::create_patch(src_format.as_str(), feature_format.as_str()); | ||||||
if diff.hunks().is_empty() { | ||||||
Ok(String::new()) | ||||||
} else { | ||||||
Ok(format!("{diff}")) | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
impl RustfmtRunner { | ||||||
fn get_binary_version(&self) -> Result<String, CheckDiffError> { | ||||||
let Ok(command) = Command::new(&self.binary_path) | ||||||
|
@@ -80,8 +106,59 @@ impl RustfmtRunner { | |||||
let binary_version = std::str::from_utf8(&command.stdout)?.trim(); | ||||||
return Ok(binary_version.to_string()); | ||||||
} | ||||||
|
||||||
// Run rusfmt with the --check flag to see if a diff is produced. Runs on the code specified | ||||||
// | ||||||
// Parameters: | ||||||
// binary_path: Path to a rustfmt binary | ||||||
// code: Code to run the binary on | ||||||
// config: Any additional configuration options to pass to rustfmt | ||||||
// | ||||||
fn format_code<'a>( | ||||||
&self, | ||||||
code: &'a str, | ||||||
config: &Option<Vec<String>>, | ||||||
) -> Result<String, CheckDiffError> { | ||||||
let config = create_config_arg(config); | ||||||
let mut command = Command::new(&self.binary_path) | ||||||
.env("LD_LIBRARY_PATH", &self.ld_library_path) | ||||||
.args([ | ||||||
"--unstable-features", | ||||||
"--skip-children", | ||||||
"--emit=stdout", | ||||||
config.as_str(), | ||||||
]) | ||||||
.stdin(Stdio::piped()) | ||||||
.stdout(Stdio::piped()) | ||||||
.stderr(Stdio::piped()) | ||||||
.spawn()?; | ||||||
|
||||||
command.stdin.as_mut().unwrap().write_all(code.as_bytes())?; | ||||||
let output = command.wait_with_output()?; | ||||||
Ok(std::str::from_utf8(&output.stdout)?.to_string()) | ||||||
} | ||||||
} | ||||||
|
||||||
fn create_config_arg(config: &Option<Vec<String>>) -> String { | ||||||
let config_arg: String = match config { | ||||||
Some(configs) => { | ||||||
let mut result = String::new(); | ||||||
result.push(','); | ||||||
for arg in configs.iter() { | ||||||
result.push_str(arg.as_str()); | ||||||
result.push(','); | ||||||
} | ||||||
result.pop(); | ||||||
result | ||||||
} | ||||||
None => String::new(), | ||||||
}; | ||||||
let config = format!( | ||||||
"error_on_line_overflow=false,error_on_unformatted=false{}", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we need to start this with
Suggested change
|
||||||
config_arg.as_str() | ||||||
); | ||||||
config | ||||||
} | ||||||
/// Clone a git repository | ||||||
/// | ||||||
/// Parameters: | ||||||
|
@@ -270,3 +347,36 @@ pub fn compile_rustfmt( | |||||
feature_runner, | ||||||
}); | ||||||
} | ||||||
|
||||||
fn search_for_rs_files(repo: &Path) -> impl Iterator<Item = PathBuf> { | ||||||
return WalkDir::new(repo) | ||||||
.into_iter() | ||||||
.filter_map(|e| e.ok()) | ||||||
.filter(|entry| { | ||||||
let path = entry.path(); | ||||||
return path.is_file() && path.extension().map_or(false, |ext| ext == "rs"); | ||||||
}) | ||||||
.map(|entry| entry.into_path()); | ||||||
} | ||||||
Comment on lines
+351
to
+360
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really happy to see that this has been abstracted out. Is there a way to combine these chained calls to |
||||||
|
||||||
pub fn check_diff(config: Option<Vec<String>>, runners: CheckDiffRunners, repo: &Path) -> i32 { | ||||||
let mut errors = 0; | ||||||
search_for_rs_files(repo).for_each(|file| match runners.create_diff(file.as_path(), &config) { | ||||||
Ok(diff) => { | ||||||
if !diff.is_empty() { | ||||||
eprint!("{diff}"); | ||||||
errors += 1; | ||||||
} | ||||||
} | ||||||
Err(e) => { | ||||||
eprintln!( | ||||||
"Error creating diff for {:?}: {:?}", | ||||||
file.as_path().display(), | ||||||
e | ||||||
); | ||||||
errors += 1; | ||||||
} | ||||||
}); | ||||||
Comment on lines
+364
to
+379
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personal preference, but I really think the code is clearer when written using a |
||||||
|
||||||
return errors; | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
use diffy::{self, create_patch}; | ||
|
||
#[test] | ||
fn diffy_test_diff() { | ||
let original = "The quick brown fox jumps over the lazy dog"; | ||
let modified = "The quick brown fox jumps over the LAZY dog"; | ||
|
||
let patch = create_patch(original, modified); | ||
// diffy uses hunks which indicates the lines that are different | ||
assert_eq!(patch.hunks().is_empty(), false); | ||
// hence regardless, patch.to_string() will never be empty | ||
assert_eq!(patch.to_string().is_empty(), false); | ||
} | ||
|
||
#[test] | ||
fn diffy_test_no_diff() { | ||
let original = "The quick brown fox jumps over the lazy dog"; | ||
|
||
let patch = create_patch(original, original); | ||
assert_eq!(patch.hunks().is_empty(), true); | ||
assert_eq!(patch.to_string().is_empty(), false); | ||
} | ||
Comment on lines
+1
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good start, and definitely helps demonstrate what we can expect to get back from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be simplified. What we need to do is make sure we add a leading comma before each user supplied option:
Sine we're concatenating the user supplied options with
error_on_line_overflow=false,error_on_unformatted=false
the result would be something like:error_on_line_overflow=false,error_on_unformatted=false,another_config=another_value