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

Allow user to specify the full name of the interface #12

Merged
merged 1 commit into from
May 21, 2016

Conversation

rakyll
Copy link

@rakyll rakyll commented May 19, 2016

goimports cannot determine which namespace you would like to import
from. Given the following command,

$ impl 'c* Conn' driver.Conn

it implements the sql/driver.Conn interface even though I would like
to implement golang.org/x/exp/io/spi/driver.Conn.

This CL allows users to specify the namespace to remove ambiguity.

$ impl 'c *Conn' golang.org/x/exp/io/spi/driver.Conn
func (c *Conn) Configure(k int, v int) error {
  panic("not implemented")
}

func (c *Conn) Transfer(tx []byte, rx []byte) error {
  panic("not implemented")
}

func (c *Conn) Close() error {
  panic("not implemented")
}

@josharian
Copy link
Owner

Thanks! I have a few nits about the implementation while I'll leave as inline comments momentarily. Also, I believe that this will fix #7.

@@ -40,8 +40,26 @@ func findInterface(iface string) (path string, id string, err error) {
return "", "", fmt.Errorf("couldn't parse interface: %s", iface)
}

var importPath string
p := strings.Split(iface, "/")
Copy link
Owner

Choose a reason for hiding this comment

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

Since all we care about is the final /, if present, I think we can do this more cheaply and simply with strings.LastIndex and some string slicing.

@josharian
Copy link
Owner

Those are all my comments. Again, thanks!

@josharian
Copy link
Owner

Also: I'm super excited about x/exp/io. Any chance there'll be native OS X support for i2c, spi, etc. via the built-in OS X FTDI USB driver? That would have made this morning about 10x nicer. 😺 (Python+libmpsse+libftdi=ugh pain on so many levels.)

@dmitshur
Copy link
Contributor

I have a comment too.

$ impl 'c *Conn' golang.org/x/exp/io/spi/driver.Conn

I've also had a need to be able to provide an exact import path and identifier to a CLI tool. I took inspiration from gorename which solved this problem before me. However, gorename needed to support more additional ways to specify identifiers, so I simplified its syntax to just a single case. See:

https://github.com/golang/tools/blob/9ae4729fba20b3533d829a9c6ba8195b068f2abc/refactor/rename/rename.go#L59

It looks like this:

"import/path".VariableName

It can be parsed as a Go expression:

https://github.com/shurcooL/vfsgen/blob/d61b62f6a1aa40a19aa6ae98c5ca0975b090ab27/cmd/vfsgendev/parse.go#L22-L55

My suggestion is to consider using that syntax so it's more similar to gorename and other tools.

@josharian
Copy link
Owner

gorename needs the quotes because it needs to be able to refer to things like methods with pointer receivers. This doesn't; interfaces always have simple names. And typing the quotes on the command line is annoying, because of the required escaping. So I'm happy with the UX for this PR.

@josharian
Copy link
Owner

I'd be OK with also accepting the quoted syntax, so that people and tools don't need to adapt specially to impl. But I'll also happily merge this PR without that; it can happen separately (and by you, if you are so inclined).

That reminds me, @jbd, a README update with this PR would be lovely.

@josharian
Copy link
Owner

Sorry, whoever I just paged, I meant @rakyll.

goimports cannot determine which namespace you would like to import
from. Given the following command,

$ impl 'c* Conn' driver.Conn

it implements the sql/driver.Conn interface even though I would like
to implement golang.org/x/exp/io/spi/driver.Conn.

This CL allows users to specify the namespace to remove ambiguity.

$ impl 'c *Conn' golang.org/x/exp/io/spi/driver.Conn
func (c *Conn) Configure(k int, v int) error {
  panic("not implemented")
}

func (c *Conn) Transfer(tx []byte, rx []byte) error {
  panic("not implemented")
}

func (c *Conn) Close() error {
  panic("not implemented")
}
@rakyll
Copy link
Author

rakyll commented May 20, 2016

All done, if you would like to add quote support, please do it after this CL is merged.

@josharian
Copy link
Owner

Thanks, @rakyll! I still have a few small nits, but rather than delaying for another back and forth, I'm going to merge this and then tweak a few things. Thanks, again.

@rakyll
Copy link
Author

rakyll commented May 21, 2016

Thanks :)

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