cmd/fillstruct: add option to select by line number.

[davidrjenni]

Fixes #3.

[arp242]

Hm, I get a "could not find file" error when I try to use -line?

[~/go/src/a]% cat a.go
package main

import (
        "net/mail"
        "strings"
)

func main() {
        _, _ = mail.Address{}, strings.Reader{}
}

[~/go/src/a]% ~/go/bin/fillstruct -offset 81 -file a.go
{"start":71,"end":85,"code":"mail.Address{\n\tName:    \"\",\n\tAddress: \"\",\n}"}

[~/go/src/a]% ~/go/bin/fillstruct -line 9 -file a.go   
fillstruct: could not find file "a.go"

[davidrjenni]

@Carpetsmoker: I updated the PR, PTAL.

[arp242]

Thanks!

[~/go/src/a]% cat a.go
package main

import (
        "net/mail"
)

func main() {
        _ = mail.Address{}
        _, _ = mail.Address{}, mail.Address{}
}

[~/go/src/a]% ~/go/bin/fillstruct -file a.go -line 9
{"start":80,"end":94,"code":"mail.Address{\n\tName:    \"\",\n\tAddress: \"\",\n}"}
{"start":96,"end":110,"code":"mail.Address{\n\tName:    \"\",\n\tAddress: \"\",\n}"}

That output for the line with two structs isn't valid JSON:

[~/go/src/a]% ~/go/bin/fillstruct -file a.go -line 9 | python -m json.tool
Extra data: line 2 column 1 (char 84)

Not sure what would be a good/sane way to handle this; put it in an array? Or maybe the current behaviour is fine and clients should treat every output line as its own JSON document?

It would also make more sense to print it in reverse, I think. Because replacing the first struct would influence the offset of what follows (making the offsets that fillstruct prints invalid). So users of fillstruct would have to loop over it in the reverse order. Not a huge deal, just a small thing ;-)

I can also give both -line and -offset:

[~/go/src/a]% ~/go/bin/fillstruct -file a.go -line 9 -offset 80
{"start":80,"end":94,"code":"mail.Address{\n\tName:    \"\",\n\tAddress: \"\",\n}"}
{"start":96,"end":110,"code":"mail.Address{\n\tName:    \"\",\n\tAddress: \"\",\n}"}

[~/go/src/a]% ~/go/bin/fillstruct -file a.go -line 9 -offset 2 
{"start":80,"end":94,"code":"mail.Address{\n\tName:    \"\",\n\tAddress: \"\",\n}"}
{"start":96,"end":110,"code":"mail.Address{\n\tName:    \"\",\n\tAddress: \"\",\n}"}

It looks like the -offset is ignored. Maybe that should be an error? Or maybe it should try to find something at the -offset first, and then fall back to -line if that fails? That would make it possible to run fillstruct on a line with two structs, and have it fill only one of them.

---

Another small thing is an inconsistency with -offset if there aren't any structs; with the same file as above:

[~/go/src/a]% ~/go/bin/fillstruct -file a.go -line 2
[~/go/src/a]% echo $?
0

[~/go/src/a]% ~/go/bin/fillstruct -file a.go -offset 2
fillstruct: no struct literal found at selection
[~/go/src/a]% echo $?
1

[davidrjenni]

@Carpetsmoker, thank you alot for your detailed feedback.

About the JSON output: fillstruct is used primarily by editor
integrations. So if the output as JSON array with the elements in
reverse order makes the integration easier, I'm happy to implement it
that way.

I like the idea of first trying to use the -offset information and,
if there was no struct literal found at that position, use the -line
information.

About the inconsistent error: when no struct literal is found: it
should definitively be consistent. I'm not sure whether it is better
to show an error or not. Also, I could not find a convention among
similar tools, e.g. honnef.co/go/tools/cmd/keyify does report an
error if there was no struct literal found at the given position,
while github.com/fatih/gomodifytags does not. I'm leaning more
towards showing an error and returning a non-zero exit code.

[davidrjenni]

@Carpetsmoker, have you had the opportunity to look at my latest changes?

[arp242]

Ugh. I don't think I got notified about that, or if I did, it slipped through. Sorry about that >_<

I'll check over the weekend!

[arp242]

Seems to work well 👍 PR for vim-go here: fatih/vim-go#1607

[davidrjenni]

Great, thanks for your feedback.

The tool now always prints a JSON array instead of a JSON object to stdout. That's a breaking change—do you think I can just merge this PR or will it break vim-go for existing users? I'm not sure how to handle that. Do you bundle vim-go releases with the accompanying tools? Or should I tag releases so that users can pull a certain version of fillstruct for a certain version of vim-go? Thanks.

[arp242]

Yeah, this PR will break vim-go's :FillStruct; it needs #1607 (which won't work with the current master).

We don't really have a strategy for managing this at the moment; :GoInstallBinaries is just a loop with go get ... Even if you tag a release, people who update vim-go will simply get an error until they run :GoUpdateBinaries, which is not exactly great UX.

People reporting issues due to outdated Go tools is actually fairly common. We may need to think about a way to do this better, like bundling dependencies or something to that effect.

Thoughts @bhcleek @fatih?

[bhcleek]

I think this is a necessary change and we should merge fatih/vim-go#1607 as soon as this PR is merged...

[davidrjenni]

OK, thank you for your feedback—I'll try to keep breaking changes at a minimum.