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

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

Merged
merged 1 commit into from
Dec 13, 2017

Conversation

davidrjenni
Copy link
Owner

Fixes #3.

@arp242
Copy link
Contributor

arp242 commented Oct 13, 2017

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 davidrjenni force-pushed the fillstruct-by-line branch 2 times, most recently from fbb6762 to e7731cf Compare October 13, 2017 13:06
@davidrjenni
Copy link
Owner Author

@Carpetsmoker: I updated the PR, PTAL.

@arp242
Copy link
Contributor

arp242 commented Oct 14, 2017

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
Copy link
Owner Author

@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
Copy link
Owner Author

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

@arp242
Copy link
Contributor

arp242 commented Dec 8, 2017

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
Copy link
Contributor

arp242 commented Dec 10, 2017

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

@davidrjenni
Copy link
Owner Author

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
Copy link
Contributor

arp242 commented Dec 12, 2017

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
Copy link
Contributor

bhcleek commented Dec 12, 2017

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

@davidrjenni
Copy link
Owner Author

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

The new command line parameter -line can be used to specify the
position of one or more struct literals by providing the corresponding
line number.

If both parameters, -offset and -line, are provided, then the tool
first uses the more specific offset information. If there was no
struct literal found at the given offset, then the line information is
used.

If more than one struct literal was found, code to fill all of them is
generated. The output is a JSON array with the elements in reverse
order of their occurence.

Fixes #3.
@davidrjenni davidrjenni merged commit cdebcee into master Dec 13, 2017
@davidrjenni davidrjenni deleted the fillstruct-by-line branch December 13, 2017 15:26
davidrjenni added a commit to davidrjenni/A that referenced this pull request Jun 2, 2018
Since davidrjenni/reftools#8, fillstruct always outputs a JSON array.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants