Skip to content
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

Implement specialized diffing for slices #131

Merged
merged 2 commits into from
Mar 12, 2019
Merged

Implement specialized diffing for slices #131

merged 2 commits into from
Mar 12, 2019

Conversation

dsnet
Copy link
Collaborator

@dsnet dsnet commented Mar 12, 2019

Lists of primitives are a common-enough data structure that
it is worth providing specialized diffing for.

This provides significantly better readability for strings
and byte slices. There is also a heuristic for detecting
what a string should be diffed as a multiline string.

Lists of primitives are a common-enough data structure that
it is worth providing specialized diffing for.

This provides significantly better readability for strings
and byte slices. There is also a heuristic for detecting
what a string should be diffed as a multiline string.
@dsnet dsnet requested a review from cybrcodr March 12, 2019 00:44
@dsnet dsnet merged commit b5cce89 into master Mar 12, 2019
@dsnet dsnet deleted the diff-slices branch March 12, 2019 03:15
list = opts.formatDiffSlice(
reflect.ValueOf(ssx), reflect.ValueOf(ssy), 1, "line",
func(v reflect.Value, d diffMode) textRecord {
s := formatString(v.Index(0).String())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me the diffed text would be a lot readable if it wouldn't be quoted and packed in strings.Join({. Is it possible to specify an option to return text like that?

I understand that the default is to produce valid go, hence the strings.Join and individual quoting. It would be interesting to know why did you take such design decision?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the other way around: would it be possible to force comparing string in one line even if it is long?

Copy link
Collaborator Author

@dsnet dsnet Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to specify an option to return text like that?

For the time being, we're avoiding the addition of options that configure how the reporter displays diffs as there's an endless number of possible outputs. The moment you add an option, you are stuck supporting it forever. We prefer to use heuristics to choose a reasonable output. It's not perfect, but hopefully good enough most of the time. If you feel the current heuristics are sub-par, it's good to give examples where it's output isn't great.

It would be interesting to know why did you take such design decision?

The reporter output prioritizes being unambiguous over being aesthetically nice. Not escaping the strings certainly looks nicer and may be easier to read in most situations, but there will be some set of strings where it is ambiguous whether the set of characters being displayed belong to the actual string itself or is part of the diff control characters (e.g., - and +).

It's possible to design an output scheme that preserve aesthetic niceness, while being ambiguous, but my fear is that this output will be unfamiliar to users. While the current output is not as aesthetically nice, it is familiar to users.

If you have an idea of how to diff strings that is: 1) always unambiguous, 2) aesthetically pleasing, and 3) reasonably familiar to someone seeing it for the first time, we're happy to consider it.

And the other way around: would it be possible to force comparing string in one line even if it is long?

By default it does compare strings using the == operator regardless of the length or contents. Did you mean for the reporter? There's a difference between how cmp compares two values and how it reports the difference between two values. As mentioned earlier, we are intentionally avoiding options to configure how the reporter output looks.

However, there is the ability to add a your own custom reporter, so if you're not fond of the current output, you can always write your own.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great that there is an option to write custom reporter. Thanks for mentioning that.

It seems to me that using + and - in diff is unambigous. It is how git diff works, so I believe it is very familiar to devs. The first char of every line is either "+", "-" or a space. Then the actual contents follow. If actual contents contain "+" as a first char in diff it will be on the second:

In below example I prepend "+" before "import".

~/Projects/handlertest$ git diff
diff --git a/body.go b/body.go
index 9b9ee88..1e51c6c 100644
--- a/body.go
+++ b/body.go
@@ -1,6 +1,6 @@
 package handlertest
 
-import (
++import (
        "bytes"
        "fmt"
        "io"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants