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

proposal: reflect: allow newlines in 'conventional' tag format #15893

Closed
dtromb opened this issue May 30, 2016 · 13 comments
Closed

proposal: reflect: allow newlines in 'conventional' tag format #15893

dtromb opened this issue May 30, 2016 · 13 comments

Comments

@dtromb
Copy link

dtromb commented May 30, 2016

(Go1.6)

The conventional tag formatting allows some kinds of whitespace in the "value" part of the tag, but not others. Specifically, it fails quietly when there is a newline:

package main

import (
    "fmt"
    "reflect"
)

type Foo struct {
   Bar string  `key:"some value with whitespace"`
   Baz string `key:"some value with 
                    a different kind
                    of whitespace"`
}

func main() {
    typ := reflect.TypeOf([]Foo{}).Elem()
    for i := 0; i < typ.NumField(); i++ {
        fmt.Printf("%d %s\n", i, typ.Field(i).Tag.Get("key"))
    }
}

The output is:

0 some value with whitespace
1 

as you can see in the Go Playground.

It does not make sense for "some whitespace but not others" to be allowed here, and this should be an easy fix. It would be very good to allow newlines here, as it would greatly improve the ability to format code without having to employ the workaround of writing one's own tag parser.

The canonical use case that I keep coming up against is embedding SQL into struct tags when I write DTO models, but one can think of dozens more....

eg.

type PersonNameModel struct {
    db *lhqm.Connection
    Create *sql.Stmt                `query:"INSERT INTO PersonName(firstname,lastname,middle,preftitle)
                                                        VALUES($1,$2,$3,$4)"`
    RetrieveById *sql.Stmt          `query:"SELECT * FROM PersonName WHERE id=$1"`
    RetreiveByLastName *sql.Stmt    `query:"SELECT * FROM PersonName WHERE lastname LIKE '%'||$1||'%'"` 
    RetreiveByFirstName *sql.Stmt   `query:"SELECT * FROM PersonName WHERE firstname LIKE '%'||$1||'%'"`
    RetreiveAltnamesByPerson *sql.Stmt      `query:"SELECT pn.* FROM PersonName pn, PersonAlternateName pan 
                                                WHERE pan.personId = $1 
                                                  AND pan.personNameId = pn.id)"`
}

The first and last fields' queries above were not being prepared by my code that iterates over the struct fields looking for *sql.Stmt fields with "query" tags! I have to put them all on a single line, or re-write the tag parser...

@ianlancetaylor ianlancetaylor changed the title Allow newlines in 'conventional' tag format. reflect: allow newlines in 'conventional' tag format May 30, 2016
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone May 30, 2016
@ianlancetaylor
Copy link
Contributor

Seems reasonable if someone wants to send a CL for the 1.8 release.

@josharian
Copy link
Contributor

If this changes, we should be sure to also update cmd/vet.

@jonbodner
Copy link

Is there still interest in changing this? Could it make 1.9?

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Unplanned, Proposal Feb 6, 2017
@bradfitz bradfitz changed the title reflect: allow newlines in 'conventional' tag format proposal: reflect: allow newlines in 'conventional' tag format Feb 6, 2017
@dtromb
Copy link
Author

dtromb commented Feb 7, 2017 via email

@jonbodner
Copy link

I'm pretty sure the change needs to be made in https://tip.golang.org/src/reflect/type.go?s=31830:31893#L1155 . Based on the comments, some code needs to be modified in other places to make sure the tests and vet still pass.

The relevant code is copied to https://play.golang.org/p/I-bUmsSJu5 .

I can take a swing at this later tonight.

@josharian
Copy link
Contributor

Thanks for looking into it. However, please hold off on working on the implementation--this is marked as a proposal, so a decision needs to get made first, and I would hate for you to end up wasting your time.

@jonbodner
Copy link

jonbodner commented Feb 7, 2017

Understand. However, it only took me a few minutes to come up with this as a proposed fix:

https://play.golang.org/p/Fcb3UR6Aw4

@rsc
Copy link
Contributor

rsc commented Feb 13, 2017

The underlying problem is that reflect defines these "" strings as being

Each key is a non-empty string consisting of non-control characters other than space (U+0020 ' '), quote (U+0022 '"'), and colon (U+003A ':'). Each value is quoted using U+0022 '"' characters and Go string literal syntax.

And Go string literal syntax using double-quotes does not admit raw newlines between the double-quotes. The playground example doesn't work because it doesn't unescape any of the expressions.

Although Go programs don't allow raw \n inside "" strings, it is in theory possible to allow them in strconv.Unquote or to have our own copy of strconv.Unquote for tag-unquoting.

Still thinking about this.

@rsc
Copy link
Contributor

rsc commented Feb 27, 2017

This seems like a fairly substantial change to make, and there's only thin justification here. How widespread is this need? There may not be sufficient justification for tweaking this.

@jonbodner
Copy link

IMHO, it's a chicken and egg problem. The uses for tags are limited because tags are limited.

The case that I'm interested in is related to a dynamic DAO generator that I wrote (https://github.com/jonbodner/proteus). Only trivial SQL queries can be reasonably be placed into a single-line struct tag. It'd be nice if longer queries could be represented in a struct tag and the
cleanest way to do so is to allow newlines in a struct tag.

@rsc
Copy link
Contributor

rsc commented Feb 28, 2017

IMHO, it's a chicken and egg problem. The uses for tags are limited because tags are limited.

We resolve this kind of problem by identifying potential use cases and places where people are struggling against the restrictions. So far there's only one such case that has been presented.

@rsc
Copy link
Contributor

rsc commented Mar 6, 2017

There doesn't seem to be sufficient justification for this, so I'm going to mark this declined. If more justification arrives, we can reopen this.

@jonbodner
Copy link

If we can't get newlines within the values of struct tags, can we get newlines between struct tag key/value pairs?

So that something like this can be done:

type Foo struct {
    Val int `json:"val"
             xml:"val"`
}

That would help with readability when there are multiple struct tag key/value pairs, especially if there are long values for one or more of them. I don't think that supporting new lines between key/value pairs would break any existing compatibility promises.

@golang golang locked and limited conversation to collaborators Mar 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants