Remove tag if new tag value is empty.#120
Conversation
findleyr
left a comment
There was a problem hiding this comment.
LGTM, with one nit. Thanks for tracking this down!
| } | ||
|
|
||
| f.Tag.Value = res | ||
| if res == "" { |
There was a problem hiding this comment.
I think it would be clearer to defer mutating f.Tag until this point. In other words, on line 104 above, just do something like
curTag := ""
if f.Tag != nil {
curTag = f.Tag.Value
}And then here, do:
if res == "" {
f.Tag = nil
} else {
if f.Tag == nil {
f.Tag = &ast.BasicLit{}
}
f.Tag.Value = res
}
The advantage of this is that we don't run the risk of leaving an empty, non-nil tag in the tree if we encounter an error. Also, it generally is easier to follow the code if state mutation is left until the end.
If the new tag value is the empty string, set the Tag to be nil. Having empty but non-nil tags causes formatting issues that leave trailing whitespace.
b7521c6 to
d38d35e
Compare
|
@fatih this is just a bug fix. If you're OK with it, can you approve? |
|
@findleyr are you OK if I add you as a collaborator so you can approve and merge PR's? |
|
@fatih sure, I can do that. I probably won't be able to offer much support, but can help fix bugs related to our usage. |
If the new tag value is the empty string,
set the Tag to be nil. Having empty but
non-nil tags causes formatting issues
that leave trailing whitespace.