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/vet: Show warning when StructTags not space-separated #9500

Closed
cjwebb opened this issue Jan 4, 2015 · 13 comments

Comments

Projects
None yet
9 participants
@cjwebb
Copy link

commented Jan 4, 2015

If StructTags are separated via tabs rather than spaces, they will not work... but go vet will not issue any warnings.

Example code (Github does not appear to render tabs and spaces differently though):

package main

import (
    "encoding/json"
    "fmt"
)

// the StructTag contains tabs, and does not recognise the json keyval
type TabsStruct struct {
    Foo string `xml:"foo1,attr" json:"foo1"`
}

// this StructTag contains only spaces, and works perfectly
type StringsStruct struct {
    Foo string `xml:"foo1,attr" json:"foo1"`
}

func main() {

    // both should print out {"foo1":"hello"}, but only the StringsStruct example does

    thing, _ := json.Marshal(TabsStruct{"hello"})
    fmt.Println(string(thing))

    thing2, _ := json.Marshal(StringsStruct{"hello"})
    fmt.Println(string(thing2))
}

Playground example: https://play.golang.org/p/DAvrKtIBKI

This issue was requested by @robpike: https://twitter.com/rob_pike/status/551313510991265792

@mikioh mikioh added the repo-tools label Jan 4, 2015

@robpike robpike self-assigned this Jan 6, 2015

@gfrey

This comment has been minimized.

Copy link

commented Jan 16, 2015

The fix for this issue (golang/tools@682ca03) introduced a new one. Something like

field type `tag:"foo bar"`

isn't working any more, due to the extra space in the quoted value string. At least in my interpretation of the docs (speaking of quoted string literals) this should be valid.

@zimmski

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2015

Bugged me too, I made a patch for this zimmski/tools@4b82084 and I will try to bring it upstream.

@zimmski

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2015

I forgot to post the CL here https://go-review.googlesource.com/#/c/2974/

@nigeltao

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2015

Please review https://go-review.googlesource.com/3952 for the
field type tag:"foo bar"
problem.

@zimmski

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2015

@robpike: I do not get it. Why does https://go-review.googlesource.com/3952 get reviewed and https://go-review.googlesource.com/#/c/2974/ does not? Is there some kind of rule I am missing?

@dsymonds

This comment has been minimized.

Copy link
Member

commented Feb 5, 2015

On 5 February 2015 at 19:28, Markus Zimmermann notifications@github.com
wrote:

@robpike https://github.com/robpike: I do not get it. Why does
https://go-review.googlesource.com/3952 get reviewed and
https://go-review.googlesource.com/#/c/2974/ does not? Is there some kind
of rule I am missing?

You never mailed 2974 for review.

@zimmski

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2015

I used git codereview mail which worked for my other CLs.

@dsymonds

This comment has been minimized.

Copy link
Member

commented Feb 5, 2015

On 5 February 2015 at 19:38, Markus Zimmermann notifications@github.com
wrote:

I used git codereview mail which worked for my other CLs.

I don't know what else to tell you, then. I can see that there is no sign
of mail being sent on your CL in gerrit. Consider filing a bug for that.

@zimmski

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2015

@dsymonds: Sorry to be a burden here, but what is the sign for mail? Because the CL is in the category "Outgoing reviews" under "My Changes" so I do not know the indication that something is missing. And what should I do with it now? Besides some test cases and the control character handling it is the same.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2015

As far as I can tell 2974 was sent out. I see it here: https://groups.google.com/forum/#!searchin/golang-codereviews/2974/golang-codereviews/hVZNIPvazNY/YISaX9QxKbkJ .

I think the main issue is that no reviewer was assigned and nobody happened to pick it up. For 3952 Nigel assigned Rob as the reviewer when he sent it out, so Rob got it in direct mail, and reviewed it quickly.

That is, CL 2974 would have been reviewed at some point, but 3952 was reviewed quickly because it was sent directly to the reviewer.

@dsymonds

This comment has been minimized.

Copy link
Member

commented Feb 5, 2015

nigeltao added a commit to golang/tools that referenced this issue Feb 6, 2015

cmd/vet: allow spaces in struct tag values.
The validateStructTag code now closely mimics the StructTag.Get code in
package reflect.

This addresses the comment raised on issue #9500:
golang/go#9500 (comment)

See also https://go-review.googlesource.com/3953

Change-Id: I583f7447dbc5a2d7ecbb393d9bb6b1515cb10b18
Reviewed-on: https://go-review.googlesource.com/3952
Reviewed-by: Rob Pike <r@golang.org>
@nigeltao

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2015

@zimmski, Ian's hypothesis sounds plausible to me.

From my point of view, I didn't realize that this bug existed (and hence that your code change was proposed) until I'd actually written the patch. (I came across this bug independently). Once I had my change (to the golang.org/x/tools repo), and the matching change to the main Go repo, it seemed faster to send both of my changes out instead of reviewing yours, especially as your new code differed from the StructTag.Get code in the standard reflect package and I'd prefer them to be similar.

Apologies for having your proposal fall through the cracks.

@nigeltao

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2015

Separately, this issue was closed by golang/tools@682ca03

@nigeltao nigeltao closed this Feb 6, 2015

@golang golang locked and limited conversation to collaborators Jun 25, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.