check_diff function rewrite
#6390
Conversation
There was a problem hiding this comment.
We're on the right track here, but I think there are a few things we need to double check.
- The
Displayoutput ofdiffy::Patchunder a few different scenarios:
- when the two
&strinputs todiffy::create_patchare the same - when the two
&strinputs todiffy::create_patchare different - The best way I can think to test this would be to create a new trait
CodeFormatterwith a single functionformat_codethat matches the definition we've already defined forRustfmtRunner::format_code. Then you can implement the trait forRustfmtRunnerusing the current definition, and makeCheckDiffRunnersgeneric overT: CodeFormatter. Should be simple enough at that point to create a unit test where you initialize aCheckDiffRunnerswith mock implementation ofCodeFormatterto test the output of callingCheckDiffRunners::create_diff
-
Invoking rustfmt correctly. We probably should write a unit test where we at least compile rustfmt from source and run it on a simple misformatted input and make sure we get back the expected output. We should also review how rust-analyzer calls rustfmt and follow a similar approach, but adapt it for the arguments we want to pass when calling rustfmt.
-
Abstracting the
.rsfiles search and unit testing it to make sure that we're picking up all.rsfiles in the current directory and any nested directories.
| impl Debug for CheckDiffError { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.debug_struct("CheckDiffError").finish() | ||
| } | ||
| } |
There was a problem hiding this comment.
Lets #[derive(Debug)] instead of manually defining it. You'll probably also need to add a #[derive(Debug)] annotation to the GitError enum.
| let output = Command::new(&self.binary_path) | ||
| .env("LD_LIBRARY_PATH", &self.ld_library_path) | ||
| .args([ | ||
| "--unstable-features", | ||
| "--skip-children", | ||
| "--emit=stdout", | ||
| config.as_str(), | ||
| code, | ||
| ]) | ||
| .output()?; |
There was a problem hiding this comment.
I think we'll need to tweak this slightly. We want to pass the code to rustfmt via stdin, and in order to do that we probably need to do something similar to rust-analyzer.
Also, I think we could simplify creating the config arg above. We should probably abstract that into a standalone function.
| let diff = diffy::create_patch(src_format.as_str(), feature_format.as_str()); | ||
| Ok(format!("{diff}")) |
There was a problem hiding this comment.
I know this was in my suggestion when messaging on Zulip. I think this should be fine, but we could also consider returning a diffy::Patch or maybe even our own wrapper type.
It would be really nice if the diff::Patch gave us some way to tell if the patch was empty, but I don't think it does so we probably need to roll our own way to figure that out.
| for entry in WalkDir::new(".").into_iter().filter_map(|e| e.ok()) { | ||
| let path = entry.path(); | ||
| if path.is_file() && path.extension().map_or(false, |ext| ext == "rs") { |
There was a problem hiding this comment.
I don't think we can call WalkDir::new("."). We need to pass the path for the repository that we want to run the check_diff function on. Remember that we're going to clone various 3rd party repos to run the check-diff on them. We should update the function parameters of check_diff to take the path we want to search in.
Also, I still think it would be great if we abstracted the .rs file search into a function that returned an Iterator of Path or PathBuf objects so that check_diff isn't responsible for that logic. Plus it'll be a lot easier to unit test that we're searching for all .rs files within a directory if we break it out.
| if path.is_file() && path.extension().map_or(false, |ext| ext == "rs") { | ||
| match runners.create_diff(path, &config) { | ||
| Ok(diff) => { | ||
| if !diff.is_empty() { |
There was a problem hiding this comment.
This is what I was getting at earlier. We need a convenient way to check if the diff is empty. I haven't worked with the diffy crate before so I don't know what it outputs when there is no diff. Does it still include the diff header like
--- a
+++ b
or is it just an empty string?
It would be great if you could investigate that / create a unit test for that behavior.
No description provided.