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

Handle import name collisions #127

Closed
wants to merge 3 commits into from

Conversation

ibuildthecloud
Copy link

@ibuildthecloud ibuildthecloud commented Jul 12, 2020

I fixed these issue before I looked to see if an existing PR was in the works, but I thought I'd submit this anyways. This should supercede #120 and #121.

Three cases are handled

  1. We maintain aliases from the source files. This is just a nicety and
    not really needed. This just makes the code prettier that is generated.
  2. Imports that conflict from multiple source files are handled.
  3. If the source file package conflict with an import. For example
package foo

import "foo"

type Example interface {
    Bad() foo.Bar
}

Using -pkg bar you would get a conflict on foo.Bar and foo.Example

Fixes #68
Fixes #94

ibuildthecloud and others added 2 commits July 11, 2020 22:54
Collisions can happen when packages are aliased, in different files, or
the local package name conflicts with an import name and a different
output package is used.
If the interface to be mocked requires an import that is shadowed by a
param with the same name, it's unlikely to be a problem for the upstream
interface (and thus go undetected), but it causes moq to generate
invalid code.

Example in the wild:
https://github.com/kubernetes/helm/blob/534193640179fe3c9ffc3012a7bc4e8b23fbe832/pkg/helm/interface.go#L28
@ibuildthecloud
Copy link
Author

I realized that this PR would break #63 so I've update this PR to include #63 plus the small change to make it work. On the positive side this should be a faster approach and more reliable(?), maybe?

@ibuildthecloud
Copy link
Author

So this should supercede #63 too. @sudo-suhas I'm sure you've busy but if you could review this PR it should close a lot of long standing issues.

@ibuildthecloud
Copy link
Author

I don't know why travis-ci never goes green in GitHub, the build is passing https://travis-ci.org/github/matryer/moq/builds/707403162

@andytsnowden
Copy link

bump @sudo-suhas any chance you could review this sometime this week? This bug is blocking our usage of the great utility you've created.

@ibuildthecloud
Copy link
Author

@sudo-suhas Pretty please look at this? I'd really like to continue to use moq, but not getting these things fixed upstream is just becoming too much. I'd rather not fork and am trying my best to do all the heavily lifting here. Please let me know if there is something I can do to help.

@sudo-suhas
Copy link
Collaborator

Hey @ibuildthecloud I completely understand and I had even taken a day off with the intention of resolving long standing issues. But I cannot merge any PRs until the Travis CI integration is fixed. I am not the owner so cannot make the necessary changes to the settings. I have requested @matryer's help to sort it out.

@sudo-suhas
Copy link
Collaborator

sudo-suhas commented Aug 16, 2020

@ibuildthecloud could you rebase this PR? That should trigger the GitHub CI (the Travis CI issue has been resolved).

@matryer
Copy link
Owner

matryer commented Sep 18, 2020

@ibuildthecloud thanks for your work on this.

@aageneralov
Copy link

What is the status of this PR? Its very useful feature

@cgorenflo
Copy link
Contributor

@ibuildthecloud please rebase so this PR can be merged. I can't wait for this to become available :)

@breml
Copy link
Contributor

breml commented Dec 15, 2020

@cgorenflo Please have a look at #141 and report if this resolves your issue as well.

@cgorenflo
Copy link
Contributor

@breml Thanks for pointing that out, works like a charm!

@damnedest
Copy link

Is it possible to merge this to master?

@matryer
Copy link
Owner

matryer commented Feb 1, 2021

This problem might be fixed now?

@sudo-suhas
Copy link
Collaborator

Yes, closed via #141.

@sudo-suhas sudo-suhas closed this Feb 2, 2021
@matryer
Copy link
Owner

matryer commented Feb 2, 2021

Thanks to @ibuildthecloud for this contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants