@sudo-suhas We have the situation described in #94. I did a test with this PR and I can confirm, that it worked.

Regarding the breaking change, could you provide an example, that illustrates when and how the breaking change could potentially affect existing (test) code.

@breml an example of the breaking change is present in one of the 'golden' files generated via tests - https://github.com/matryer/moq/pull/141/files#diff-f8cb62335f3c482f132a362ff53cecc339a89fabfdc2ed25539b283308509678R36-R39

@breml What do you think about this? I'm not familiar with the key differences between this PR and #94.

@matryer this PR fixes the issue reported in #94. There are other PRs which also attempt to do the same. The difference would be that this introduces some abstraction and splits the Mocker into more manageable pieces rather than trying to fit the disambiguation logic for imports and variables into the existing type.

@matryer I spent some time to have a more in depth look at this PR.

TLDR: The result seems good, at least it worked for me. The PR as a whole is really difficult to review.

I have the following thoughts about this PR:

• Generally, I think the the improvements, that are introduced with this PR are very important for moq. I also think, the design decision to use a registry to keep track of the used names is a good one. So from this point of view, I think this PR is a very good step for moq to solve these issues.
• As mentioned in my previous comment, it solves issue Package name redeclared when shadowing packages and using pkg flag #94 for me in a particular situation I had the other week. In the past I now and then observed similar problems especially with name clashes between packages and parameter names. I normally worked around the problem by choosing different names for the parameters to prevent the name clash. I think, I also observed the situation in Import aliases not maintained, cannot import multiple packages with same name #68.
• From a review point of view, this PR is really difficult, because it changes a lot in a single commit. For example I would like to have the tests and the changes to moq in a separate commit, such that I as a reviewer can easily follow the changes made and reason about the effects of the change. This PR combines multiple things like refactoring the package/directory structure (introduction of internal package), adding the registry, introduce templates, introduce breaking changes to the generated mocks, moving tests to a new format (explicit tests to golden files) and so on. So from this point of view, I would wish, these changes are split into multiple commits or event PR. A good example for this is pkg/moq/moq.go. There are so many changes to this single file, I do not have the time to really go through and understand each change.
• I am not really sure about the decision to use the type information for the generated variable names. To prefix variable names with a type dependent prefix does look like code smell to me, especially in a statically typed language. I think there are other ways to address the name clash issue. That being said, I like the MoqParam suffix. So maybe this could have been handled all the way down by using this (or a similar) suffix to moq-ize the variable names.
• The new packages internal/registry and internal/template do not really have coverage by unit tests, so this makes it even more difficult to get a good feeling about the changes introduced by this PR. Chances are, that these packages are covered by the tests in pkg/moq, but again, this is hard to tell.

With all that said, to me the question is, are we willing to merge this PR to fix the before mentioned issues while taking the risk, that other things might be broken, due to the difficulty to properly review this PR and due to the missing tests/coverage for certain areas of the code.

@breml Thank you for taking the time to review the changes. I appreciate it very much. I do have some clarifications though.

I would like to have the tests and the changes to moq in a separate commit, such that I as a reviewer can easily follow the changes made and reason about the effects of the change... moving tests to a new format (explicit tests to golden files)_and so on.

The golden file tests were added some time back. While I was adding new tests, I moved most of the existing golden file tests into a single table-driven one. I have added a commit which refactors the existing tests in a separate commit.

This PR combines multiple things like refactoring the package/directory structure (introduction of internal package), adding the registry, introduce templates, introduce breaking changes to the generated mocks ...

I agree that the PR does a lot of changes. That's partly because I was (and am) not sure how to split some of the changes. For example, I am not sure how I can split the internal/registry+internal/template refactor because the template has to reference the registry types to reflect the changes made while resolving conflicts. While it is no way a justification, I think you would be able to understand that there was also a limit on the time I could dedicate for doing the changes.

A good example for this is pkg/moq/moq.go. There are so many changes to this single file, I do not have the time to really go through and understand each change.

With the introduction of internal/registry, I don't think the changes in pkg/moq/moq.go could have been avoided. That's not to downplay the difficulty of reviewing the changes. Instead, perhaps, it would be better to view only the resulting file. I do believe that the file now is a lot simpler to read since a lot of the heavy lifting is moved to the registry.

I am not really sure about the decision to use the type information for the generated variable names. To prefix variable names with a type-dependent prefix does look like code smell to me, especially in a statically typed language. I think there are other ways to address the name clash issue.

The generated names for method parameters is used only if there wasn't one defined in the interface definition (or was _) to begin with(see interface, moq). The type names are not prefixed to variable names otherwise. And with regards to the generated names when none were present, I would argue that they are still better than in1/in2/in3.... And name clashes with imported packages are resolved by appending MoqParam. Name clash with variables is resolved by a serial number suffix (myName1,myName2 etc.).

The new packages internal/registry and internal/template do not really have coverage by unit tests, so this makes it even more difficult to get a good feeling about the changes introduced by this PR.

This is mostly because I am not sure how I can test these. Consider registry.Var - how do I generate an instance of types.Var with the appropriate type, package etc.? I am not sure. If I were to try to use the testpackages for this, that would not be very different from the current tests in pkg/moq.

All that said, I am still open to listing the specific steps we could take to make the PR easier to review and also improve the confidence for merging in the changes. But I would need your help to figure out what those steps need to be.

@sudo-suhas Thanks for your reply. As mentioned already in my first review comment, I think this PR brings a lot of improvement. I consider myself a regular user of moq and I contributed some smaller PR in the past. But I am not familiar with the whole code base of moq in depth. @matryer invited me to share my thoughts and this is what I did. No offence intended.

I spent again some time on this PR and I have the following additional input for you:

Generate an instance of types.Var:

This can be as follows:

import "go/types"

var x = types.NewVar(token.NoPos, nil, "variablename", types.Universe.Lookup("int").Type())

sync package edge case

For the following interface, the generation of the mock failed:

package syncimport

import (
	stdsync "sync"

	sync "github.com/matryer/moq/pkg/moq/testpackages/syncimport/sync"
)

type Syncer interface {
	Blah(s sync.Thing, wg *stdsync.WaitGroup)
}

Where package github.com/matryer/moq/pkg/moq/testpackages/syncimport/sync is:

package sync

type Thing string

Maybe I came across as defensive 🙈 but I was not offended in the least. I will find some time to incorporate the helpful input you have provided.

I made some improvements to the tests to further improve coverage:

Hope this is sufficiently high coverage for the internal/{registry, template} packages.

edit: Not sure if it was understood, the sync package edge case has been covered as well.

@sudo-suhas Would you be up for a quick 15 minute screenshare some time to go through this? It looks like you've done a lot of work here, and I want to make sure I understand the new design, and your thinking behind it.

Sure @matryer, can do. Should we chalk out the time and tool via Twitter DMs?

@matryer, @sudo-suhas Did it workout with your direct chat about this PR? What are the conclusions?

No, not yet. We will try to sort it out soon.

@matryer I have created a deck which

• enumerates the problems that this PR tries to solve
• compares with existing PRs
• walks through the changes being done in this PR.

Please check it out - https://docs.google.com/presentation/d/1-HVOygCVpQQ_8ikE2_OYzlvuH1NYI3wT02PWn0Onpcc/edit?usp=sharing.

This is great work. And the overview by @sudo-suhas is outstanding. Thank you.

Thank you @matryer for your kind words 🙏🏼

With regards to the tagged release v0.1.6, since this was a breaking change, it should have been v0.2.0, CMIIW.

@sudo-suhas ahh yeah - that's a good point. Do you think it's worth removing the release and retagging? Or has that ship sailed now?

Technically, moq hasn't yet reached v1, but the last thing we want is to break things for those using this tool.

The correct way to handle it would be to release v0.1.7 which would be equivalent to v0.1.4 (what happened to v0.1.5? 🙈) and then release v0.2.0 which would be equivalent to v0.1.6.

What's the advantage of bumping the minor release? Is this respected by tools, or just a better signal to moq users?

v0.1.5 got borked and I didn't want to risk having a bad release. 😬

To the best of my understanding, for v0 semantic versioning, bumping the minor version indicates a breaking change while incrementing the patch version is expected to add new backwards-compatible features or bug fixes.

Oh right, in modules you mean? Yeah ok, it's worth doing then.

I'm away from my computer for a bit, do you have the right permissions to do a release?

Have created both releases as discussed.

Conclusion: Use moq v0.2.0 to get the changes made in this PR.

Thanks @sudo-suhas and @matryer for the great work, this is very much appreciated.