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

Adopt cmp.Diff for showing unmatched arguments #154

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SpencerC
Copy link

Migrating from: golang/mock#647 (comment)

@CLAassistant
Copy link

CLAassistant commented Feb 19, 2024

CLA assistant check
All committers have signed the CLA.

@SpencerC SpencerC force-pushed the cmp_diff2 branch 2 times, most recently from a37ae2a to 6e34dff Compare February 22, 2024 20:05
@SpencerC
Copy link
Author

@r-hang @sywhang @JacobOaks, what's needed to get this PR into shape for approval?

@SpencerC
Copy link
Author

SpencerC commented Jul 5, 2024

@r-hang @sywhang @JacobOaks LMK, I see other PRs going in.

@r-hang
Copy link
Contributor

r-hang commented Jul 10, 2024

Hey @SpencerC, thanks for the contribution and sorry for the delay.

While I understand that the go-cmp library has lots of users and powerful features, I think that having this mocking library expose another repository's types as part of its public interface could make it harder to maintain as we're tightly coupled with go-cmp. Is there a way for this library to own all of its exported types?

@SpencerC
Copy link
Author

SpencerC commented Jul 10, 2024

Hey! Here are the options I see for avoiding exporting go-cmp arguments:

  1. Export wrapper type analogs for the 8 cmp.Options, and convert them to go-cmp types under the hood.
  2. Vendor-in or rewrite the diff computing functionality from the current version of go-cmp and maintain it yourself going forward.

Can you think of any other options? I'm happy to do either of those if you think they're really preferable to exporting go-cmp types.

I definitely agree with you in principle on maintaining minimal reliance on exported external deps. I feel like in the case of go-cmp though it's so established and so stable that it might as well be treated like a built-in type for the purposes of maintaining a library (like the golang.org/x/... packages used elsewhere in this library). Likewise, if users pose questions about how to properly configure rich diffs you can just say "refer to the go-cmp docs, we just pass those options through".

@r-hang
Copy link
Contributor

r-hang commented Jul 12, 2024

Hey @SpencerC, I'm personally open to using wrapper type analogs for go-cmp options, I think that's more favorable than vendoring go-cmp in.

Instead of initially adding 8 options, I think it makes sense to make go-cmp's drive diffing presentation for unmatched arguments and then port over the options individually as needed. What do you think?

@SpencerC
Copy link
Author

@r-hang, sorry just realized I didn't reply. That sounds fine to me, will just take some time for me to get back to this. Thanks!

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.

3 participants