feat: Add diff mode#112
Conversation
| if err != nil { | ||
| t.Errorf("Had trouble running keep-sorted --mode diff: %v", err) | ||
| } | ||
| if diff := cmp.Diff(string(wantDiff), gotDiff); diff != "" { |
There was a problem hiding this comment.
I'm not sure I see a whole lot of value in writing down both the out and diff files. They should theoretically be derivable from each other
What do you think about instead of doing the comparison like this, we instead apply the output of the diff operation to the input and make sure we get the out file?
There was a problem hiding this comment.
Agree, done. Also, I've extracted diff tests into a subtest.
Closes google#111
|
@JeffFaer, thank you for your review and great comments. I've updated the PR. P.S. Sorry for the delay. |
| files, _, err := gitdiff.Parse(strings.NewReader(gotDiff)) | ||
| if err != nil { | ||
| t.Fatalf("Had trouble parsing diff: %v", err) | ||
| } | ||
| if len(files) != 1 { | ||
| t.Fatalf("Exactly one file is expected in diff, got %d", len(files)) | ||
| } | ||
| var b strings.Builder | ||
| err = gitdiff.Apply(&b, bytes.NewReader(in), files[0]) | ||
| if err != nil { | ||
| t.Fatalf("Had trouble applying diff: %v", err) | ||
| } |
There was a problem hiding this comment.
Couldn't this just be ubdiff.ApplyUnified(gotDiff, in)?
There was a problem hiding this comment.
If you are talking about aymanbagabas/go-udiff@v0.4.1, it fails to apply unified with context lines, see https://github.com/aymanbagabas/go-udiff/blob/v0.4.1/unified.go#L301
There was a problem hiding this comment.
Gotcha. I'm not a huge fan of adding a second dependency just to apply diffs, even if it is test-only...
I can think of a couple different options here:
- Edit the upstream to handle context lines correctly. It seems like it would be relatively easy to add support for that, although I'm not sure if the gotools package really needs that functionality which might make it a harder sell. And this option will take longer than the next option...
- We could change our interaction with udiff to make it configurable from this test
- Instead of using
udiff.Unified, we would useudiff.Linesto calculate[]Edits and useudiff.ToUnifiedDiffwhich has a configurablecontextLinesparameter - We add a hidden flag that this test can use to override the number of context lines to 0. That lets
udiff.ApplyUnifiedwork. There's already some precedent for hidden, test-only flags
- Instead of using
- Find some other package that both calculates and applies diffs. Although I do like how the udiff package is just a mirror of a gotools package...
2 participants
This PR adds new
--mode diffand closes #111Example: