fatih/gomodifytags

Wrong output when the file isn't gofmt'd

arp242 opened this issue · 7 comments

arp242 commented

With the following file:

$ cat tags.go
package a
type x struct {
    Foo int
    bar int
}

The tool gives the wrong results.

Should be start=2:

$ gomodifytags -format json -file tags.go -transform snakecase -line 3,4 -add-tags json -add-options json=
{
  "start": 3,
  "end": 4,
  "lines": [
    "type x struct {",
    "\tFoo int `json:\"foo\"`"
  ]
}

Should be start=2, and one line is missing from the output:

$ gomodifytags -format json -file tags.go -transform snakecase -line 3,3 -add-tags json -add-options json=
{
  "start": 3,
  "end": 3,
  "lines": [
    "type x struct {"
  ]
}

start is correct, but the last line (}) missing:

$ gomodifytags -format json -file tags.go -transform snakecase -offset 23 -add-tags json -add-options json=
{
  "start": 2,
  "end": 5,
  "lines": [
    "",
    "type x struct {",
    "\tFoo int `json:\"foo\"`",
    "\tbar int `json:\"bar\"`"
  ]
}

It seems that the tool operates on the file as if they're gofmt'd.

arp242 commented

The problem is that it parses the file, does the modifications, and then calls format.Node() on the result which gofmt's the source. It then bases the offsets on the result, rather than the original.

I'm not sure if there a straight-forward way to print out an ast tree with the positions as they were found in the original source.

arp242 commented

There's another issues due to the same cause; say you have:

type foo struct {
	foo string
}

And now you add another field, and end up with (note the blank line before }):

type foo struct {
	foo string
	bar string

}

You use :GoModifyTags, but it returns offset is not inside a struct.

fatih commented

Hi @Carpetsmoker

Thanks for the bug report. I've tried to fix it via the SourcePos feature of the printer and added your initial three test cases (#41). It passes the test cases now (line offset starts at 1 so it's correct that they're returning 3 instead of 2.)

However the --json mode is a core feature and a lot of people rely on it, not sure if this fixes all cases or introduces new issues. I want to add more test cases before merging it.

fatih commented

Alright I think all your cases are now fixed and it works great. I've tested it now with many options and also added a lot of test case (including all yours listed above) in #41. I'm going to use a local version for a while and then merge it. You can also try out #41 as well if you wish.

arp242 commented
arp242 commented

Seems to work well thus far; only thing I noticed is that the indentation of the struct fields isn't always but correct, but that's not a huge deal (certainly a lot better than before!)

fatih commented

@arp242 I think the go/parser has a bug when SourcePos is enabled. It strips \t characters or inject them. In the end it should not affect the because it's syntactically correct. I'll merge the PR, thanks for the feedback again.

  • 👍1