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

Readability of cmp.Diff function #328

Open
amirrmonfared opened this issue Mar 31, 2023 · 7 comments
Open

Readability of cmp.Diff function #328

amirrmonfared opened this issue Mar 31, 2023 · 7 comments
Labels
reporter Improvements in the difference reporter

Comments

@amirrmonfared
Copy link

I believe we can make the readability of the Diff function output very much by just adding (-want +got)

@mrwormhole
Copy link

mrwormhole commented Apr 21, 2023

for the love of god, please @dsnet @neild

I still don't understand why cmp.Diff takes x and y, I have seen dozens of mistakes of mixing want and got (at least x means want and y means got, could there be a renaming for those pls)

@dsnet
Copy link
Collaborator

dsnet commented Apr 21, 2023

I don't think this is going to change. This was discussed in the early days of cmp deployment, and people could not agree whether got or want comes first. People were relatively evenly split between both. In the lack of a uniform consensus, we went with an approach that avoids choosing a path.

Feel free to create a package that simply wraps cmp.Diff and injects the headers as you see fit.

@dsnet
Copy link
Collaborator

dsnet commented Apr 21, 2023

For the record, I was in support of (-got +want), but there were a number of notable people on the Go team who instead argued for (-want +got). There are valid arguments for both:

  • For (-got +want), the argument is that it matches the conventional ordering of "got" appearing before "want".
  • For (-want +got), the argument is that it displays a diff of how the expected value was changed into the current value (i.e., what changed relative to the expected value).

The Go style guide remains ambiguous about what direction to use. Although, it seems to suggest want, got, but also seems to permit got, want.

@neild
Copy link
Collaborator

neild commented Apr 21, 2023

And also, while "got" and "want" make sense in tests, Diff output isn't necessarily used only in tests.

Perhaps there should be a test-specific diff API with named parameters:

diff := cmp.C{
  Want: want,
  Got:  got,
}.Diff(opts)

@mrwormhole
Copy link

mrwormhole commented Apr 21, 2023

very tricky, looks like it is not settled;

@dsnet
Copy link
Collaborator

dsnet commented Apr 21, 2023

When it comes to cmp.Equal it doesn't matter since the cmp guarantees symmetric property of equality.

@mrwormhole
Copy link

mrwormhole commented Apr 21, 2023

I found cmp.Diff example in google styling guide, I think it can be enforced pretty well if not, should be suggested on README.md perhaps, I will also check out other testing libraries and come back with my findings for - + or + -

https://google.github.io/styleguide/go/best-practices#tests

but you are absolutely right equality shouldn't care about want and got, it takes 2 numbers and return bool

perhaps at the end of the day + - and - + doesn't matter at all, just like yaml or yml where you stick one and in that repo everyone uses that side (aka eventual consistency)

@dsnet dsnet added the reporter Improvements in the difference reporter label Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reporter Improvements in the difference reporter
Projects
None yet
Development

No branches or pull requests

4 participants