Skip to content
This repository was archived by the owner on Jun 27, 2023. It is now read-only.

Fix variadic mock for gomock.Any() . fix #68 #101

Merged
merged 5 commits into from
Oct 1, 2017

Conversation

hori-ryota
Copy link

@hori-ryota hori-ryota commented Aug 18, 2017

Please see #68 .

@hori-ryota
Copy link
Author

Sorry, broke other tests. I'll fix.

@hori-ryota
Copy link
Author

fixed.

@balshetzer balshetzer assigned balshetzer and unassigned balshetzer Aug 24, 2017
@hori-ryota
Copy link
Author

@balshetzer how about this?

@hori-ryota
Copy link
Author

fmm...

@balshetzer
Copy link
Collaborator

Hi. This change is good but I wanted to clean it up a bit. The first error message is not the same as the others, for example.

@hori-ryota
Copy link
Author

OK. Is something better for me to do?

@hori-ryota
Copy link
Author

@balshetzer Can I fix it? Let me know what you want to fix and I will try!

@balshetzer
Copy link
Collaborator

@hori-ryota The only mandatory thing I want fixed is for the error message on line 203 to be clearer. It should be similar to the other error messages.

Some nice to have improvements:

  • the error case mentioned above should probably fail when the EXPECT call is first created.
  • there are many error cases here. Can they be reduced?
  • The condition on lines 222-223 is complex and not obvious. It might be good to have some comments with examples to explain the various situations it covers.
  • alternatively, could the matching be rewritten to first handle all non-variadic args and then have a separate case to handle the variadic args? That might make the code easier to read.

@hori-ryota
Copy link
Author

How about my refactor?

@balshetzer balshetzer merged commit 4887ba1 into golang:master Oct 1, 2017
@balshetzer
Copy link
Collaborator

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants