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

GoImpl: support relative (vendor) imports #1322

Merged
merged 1 commit into from
Sep 11, 2017
Merged

GoImpl: support relative (vendor) imports #1322

merged 1 commit into from
Sep 11, 2017

Conversation

boz
Copy link
Contributor

@boz boz commented Jun 7, 2017

This fixes :GoImpl for cases when the interface package is in vendor or similar. It depends on josharian/impl#18, which adds the -dir argument (-dir defaults to $PWD so hopefully the fallback won't be used much).

if go#util#ShellError() != 0
call go#util#EchoError(result)
return
end
endif
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we run another if this fails. If you could give me an example why it fails I can see why you did this. Why if we always pass the -dir? What exactly does this fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The current version of the impl command ignores vendor when finding the interface to generate. It will fail if the interface is installed in vendor but not $GOPATH.
  • Support vendor imports josharian/impl#18 adds -dir <dir> to root the normal go package resolution rules at <dir>, which fixes the vendor issue. <dir> defaults to $PWD.
  • The current version of impl does not take a -dir <dir> argument.

Maybe an example will help. impl.new is from josharian/impl#18 and impl.orig is the current release:

$ mkdir -p $GOPATH/src/test.com/user/vim-go-impl
$ cd $GOPATH/src/test.com/user/vim-go-impl
$ mkdir -p vendor/src/test.com/foo
$ cat > vendor/src/test.com/foo/foo.go <<EOF
> package foo
> type I interface {
>   Bar()
> }
> EOF
$ impl.orig 'i *I' foo.I
interface foo.I not found: couldn't find package src/test.com/foo: cannot find package "src/test.com/foo" in any of:
  /usr/local/Cellar/go/1.8.3/libexec/src/src/test.com/foo (from $GOROOT)
  /Users/abozanich/gopath/src/src/test.com/foo (from $GOPATH)
$ impl.new 'i *I' foo.I
func (i *I) Bar() {
  panic("not implemented")
}

$ cd ..
$ impl.new 'i *I' foo.I
unrecognized interface: foo.I
$ impl.new -dir $PWD/vim-go-impl 'i *I' foo.I
func (i *I) Bar() {
  panic("not implemented")
}

$ impl.orig -dir $PWD/vim-go-impl 'i *I' foo.I
impl <recv> <iface>

impl generates method stubs for recv to implement iface.

Examples:

impl 'f *File' io.Reader
impl Murmur hash.Hash

Don't forget the single quotes around the receiver type
to prevent shell globbing.

It runs another if it fails because I didn't want to break installs with an older impl build. Omitting the -dir <dir> is usually ok for my workflow because I'm generally cd'd into the directory that has vendor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this workaround now that the impl PR is merged?

If you could rebase on master then I can test and merge it.

  * attempt to use `-dir` argument from josharian/impl#18 if `impl`
    fails.
  * backwards compatible with previous `impl` versions.
@arp242
Copy link
Contributor

arp242 commented Sep 8, 2017

Could you tell me how to reproduce this issue? It's not really clear from this PR or the impl PR.

On master (without this patch) I tried using:

:GoImpl l Log github.com/sirupsen/logrus.StdLogger

Which seems to work correct?

This package is vendored for the project I have open. I made sure it's not in my regular GOPATH.

What am I doing wrong?

@boz
Copy link
Contributor Author

boz commented Sep 8, 2017

@Carpetsmoker: impl defaults to -dir $PWD. I wrote an example here, take a look at what happens after the cd ...

@arp242
Copy link
Contributor

arp242 commented Sep 10, 2017

Ah right, didn't see that because it was collapsed 👍

@arp242 arp242 merged commit 75d0e9f into fatih:master Sep 11, 2017
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