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

Jump to bottom

cmd/vet: Show warning when StructTags not space-separated #9500

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

cmd/vet: Show warning when StructTags not space-separated #9500

cjwebb opened this issue Jan 4, 2015 · 13 comments
Labels
FrozenDueToAge

Comments

@cjwebb
Copy link

cjwebb 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
@robpike robpike self-assigned this Jan 6, 2015
@gfrey
Copy link

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

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

zimmski commented Jan 29, 2015

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

@nigeltao
Copy link
Contributor

nigeltao commented Feb 5, 2015

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

@zimmski
Copy link
Contributor

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

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

zimmski commented Feb 5, 2015

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

@dsymonds
Copy link
Contributor

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

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

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.

@minux minux mentioned this issue Feb 5, 2015
Closed
@dsymonds
Copy link
Contributor

dsymonds commented Feb 5, 2015 via email

nigeltao added a commit to golang/tools that referenced this issue Feb 6, 2015
@nigeltao
cmd/vet: allow spaces in struct tag values.
9622599 
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
Copy link
Contributor

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

nigeltao commented Feb 6, 2015

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

@nigeltao nigeltao closed this as completed Feb 6, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
@rsc rsc unassigned robpike Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@dsymonds @mikioh @cjwebb @gfrey @zimmski @ianlancetaylor @robpike @nigeltao @gopherbot