search

uber-go / mock

GoMock is a mocking framework for the Go programming language.
Apache License 2.0
2.33k stars 121 forks source link

fix: import and arg collision #219

Closed bstncartwright closed 3 weeks ago

bstncartwright commented 1 month ago

Resolves #218

This is my first time contributing to this project, please let me know if there is anything I need to change. Thanks!

HassanCehef commented 1 month ago

I'm not a maintainer; I reported the bug.

I like the fix. Name collisions have always been possible: they didn't happen so much because "arg0"..."arg5" are not usual package names. How about using this as a suffix rather than a full replacement? DoThat(internalpackage int): instead of becoming 

-func (m *MockMything) DoThat(arg0 int) internalpackage.FooExported {
+func (m *MockMything) DoThat(internalpackage_arg0 int) internalpackage.FooExported {

this way we still get the v0.5.0 feature that provides human readable names when using mocks in all cases.

bstncartwright commented 4 weeks ago

I'm not a maintainer; I reported the bug.

I like the fix. Name collisions have always been possible: they didn't happen so much because "arg0"..."arg5" are not usual package names. How about using this as a suffix rather than a full replacement? DoThat(internalpackage int): instead of becoming

-func (m *MockMything) DoThat(arg0 int) internalpackage.FooExported {
+func (m *MockMything) DoThat(internalpackage_arg0 int) internalpackage.FooExported {

this way we still get the v0.5.0 feature that provides human readable names when using mocks in all cases.

Interesting, I like that idea but I wonder if it could cause more confusion. Auto-complete from the LSP could show internalpackage_arg0 and the developer might think it is an argument of that package? Just a thought, I am open to finding the best answer here!

bstncartwright commented 3 weeks ago

Added a comment. Also, could you re-generate existing test mocks?

Done. Thanks for your review!

becoded commented 1 week ago

@tchung1118 can we expect a new patch/minor release for this fix anytime soon?