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: support tab and newline as tag separator #38641

Closed
wuhuizuo opened this issue Apr 24, 2020 · 15 comments
Closed

proposal: reflect: support tab and newline as tag separator #38641

wuhuizuo opened this issue Apr 24, 2020 · 15 comments

Comments

@wuhuizuo
Copy link

Sometime structs are defined with mutlti tags, and long then 100 or 120 columns.
The code can be formated pretty if struct tag support spliting with blank or "\n" or "\t".

// Current record struct
type Current struct {
	ID   int32  `db:"id,type=INT,auto_increment,key"        binding:"gte=0"                   json:"id,omitempty"    form:"id"     uri:"id"`
	Flag int32  `db:"flag,type=INT,not_null,primary"        binding:"gte=0"                   json:"flag,omitempty"  form:"flag"`
	OS   string `db:"OS,type=VARCHAR(16),not_null,primary"  binding:"omitempty,oneof=AND IPH" json:"OS,omitempty"    form:"OS"     example:"AND"`
	// ......
}

// Expect record struct, supporting one tag each line
type Expect struct {
	ID int32   `db:"id,type=INT,auto_increment,key"
                binding:"gte=0"
			    json:"id,omitempty"
			    form:"id"
			    uri:"id"`
	Flag int32 `db:"flag,type=INT,not_null,primary"
				binding:"gte=0"
				json:"flag,omitempty"
				form:"flag"`
	OS string  `db:"OS,type=VARCHAR(16),not_null,primary"
				binding:"omitempty,oneof=AND IPH"
				json:"OS,omitempty"
				form:"OS"
				example:"AND"`
	// ......
}

each tag value can get by parsing the sub string in "" part after key:

similar with issue: #15893, but here: newline supporting between tags.

@ianlancetaylor ianlancetaylor changed the title reflect support tag splitor: "\n", "\t" proposal: reflect: support tab and newline as tag separator Apr 25, 2020
@gopherbot gopherbot added this to the Proposal milestone Apr 25, 2020
@ianlancetaylor
Copy link
Contributor

Thanks. I turned this into a proposal.

@ianlancetaylor
Copy link
Contributor

Although now that I've done that, I see that this is basically the same as #15893, which you mentioned. What is the difference between this proposal and that one?

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 25, 2020
@wuhuizuo
Copy link
Author

Although now that I've done that, I see that this is basically the same as #15893, which you mentioned. What is the difference between this proposal and that one?

#15893 is about newline supporting in one tag's value, here is spliting for multi tags with newline chars.

@ianlancetaylor ianlancetaylor removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 28, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Apr 28, 2020
@rsc
Copy link
Contributor

rsc commented Apr 29, 2020

In general I like the implicit backpressure that the single-line requirement imposes: it keeps these things from turning into arbitrary pages-long annotation texts. I am not convinced we really want to open that door. The examples in the top comment are right at the limit of what seems reasonable.

If we did allow multiline strings I guess gofmt would need to adjust the indentation of the continuation lines when it adjusts the padding of fields?

@rsc
Copy link
Contributor

rsc commented May 27, 2020

Ultimately this comes down to a judgement call, not really a cost-benefit. But there is a real benefit to pushing back on such long struct tags - they persist at runtime in the binary, and they complicate trying to understand the program.

Most uses of struct tags in the Go ecosystem are very short. Personally, I still believe that we should not encourage them to be very long, nor make that easier.

We definitely cannot make gofmt align the multiline formatting (inserting or removing spaces inside the backquoted string), by the way, because in general gofmt can't be sure about the interpretation of these strings.

/cc @myitcv @ianthehat @mvdan for more tools experience in this discussion.
(Please feel free to cc more people.)

@rsc rsc moved this from Incoming to Active in Proposals (old) May 27, 2020
@mvdan
Copy link
Member

mvdan commented May 27, 2020

I agree with the last comment. Multiline string literals can also easily make struct types confusing or less readable, as there wouldn't be an indentation/format style that could be enforced.

I think Go wouldn't have used a string literal for tags if they wanted tags to support complex data. It's really meant for a few simple attributes. If we really wanted we could add syntax for structural tags, akin to composite literals, but I imagine that would also encourage abusing tags for multiple purposes.

@carnott-snap
Copy link

Spitballing alternatives, would it be reasonable to have multiple field tags? Then the compiler count combine them with " ". This would serve to simplify the alignment issues, since there is a way that these should be formatted/spaced, but would both change the syntax of the language, and probably make hell with the parser.

// Current record struct.
type Current struct {
 ID   int32  `db:"id,type=INT,auto_increment,key"`       `binding:"gte=0"`                   `json:"id,omitempty"`   `form:"id"`   `uri:"id"`
 Flag int32  `db:"flag,type=INT,not_null,primary"`       `binding:"gte=0"`                   `json:"flag,omitempty"` `form:"flag"`
 OS   string `db:"OS,type=VARCHAR(16),not_null,primary"` `binding:"omitempty,oneof=AND IPH"` `json:"OS,omitempty"`   `form:"OS"`   `example:"AND"`
 // ......
}

// Expect record struct, supporting one tag each line.
type Expect struct {
 ID int32   `db:"id,type=INT,auto_increment,key"`
            `binding:"gte=0"`
            `json:"id,omitempty"`
            `form:"id"`
            `uri:"id"`
 Flag int32 `db:"flag,type=INT,not_null,primary"`
            `binding:"gte=0"`
            `json:"flag,omitempty"`
            `form:"flag"`
 OS string  `db:"OS,type=VARCHAR(16),not_null,primary"
            `binding:"omitempty,oneof=AND IPH"`
            `json:"OS,omitempty"`
            `form:"OS"`
            `example:"AND"`
 // ......
}

@rsc
Copy link
Contributor

rsc commented Jun 3, 2020

@carnott-snap even if adjacent back-quoted strings were somehow combined, splitting them across lines would not work because there would be an implicit semicolon after each string-at-end-of-line.

(And you can't change the implicit semicolon after a string without breaking x = `a`.)

Thanks for adding your opinion @mvdan.

Based on the discussion above, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jun 3, 2020
@carnott-snap
Copy link

carnott-snap commented Jun 3, 2020

@carnott-snap even if adjacent back-quoted strings were somehow combined, splitting them across lines would not work because there would be an implicit semicolon after each string-at-end-of-line.

I am unclear how that invalidates the example, you could just update the parser to handle it. If your argument is that would be too expensive, then this becomes a readability vs parser elegance argument. If the lack of a block is troublesome, you could fix that too:

// Expect record struct, supporting one tag each line.
type Expect struct {
 ID int32 {
            `db:"id,type=INT,auto_increment,key"`
            `binding:"gte=0"`
            `json:"id,omitempty"`
            `form:"id"`
            `uri:"id"`
 }

 Flag int32 {
            `db:"flag,type=INT,not_null,primary"`
            `binding:"gte=0"`
            `json:"flag,omitempty"`
            `form:"flag"`
 }

 OS string {
            `db:"OS,type=VARCHAR(16),not_null,primary"
            `binding:"omitempty,oneof=AND IPH"`
            `json:"OS,omitempty"`
            `form:"OS"`
            `example:"AND"`
 }

 // ......
}

@mvdan
Copy link
Member

mvdan commented Jun 3, 2020

@carnott-snap the rules for semicolon injection are part of the syntax, declared in the spec. Changing them isn't just about updating one parser or lexer. All Go developers have to adapt to the new rules, tools have to be fixed and updated, etc.

It is indeed possible to propose changes to the Go language, but the bar for them is high, and you must fill a template to provide enough information for the reviewers: https://github.com/golang/proposal/blob/master/go2-language-changes.md

@carnott-snap
Copy link

I am not suggesting that the semicolons be removed, but that the parser understand that these are equivalent:

one   int32  `db:"id,type=INT,auto_increment,key"`  `binding:"gte=0"`  `json:"id,omitempty"`  `form:"id"`  `uri:"id"``
two   int32  `db:"id,type=INT,auto_increment,key"`; `binding:"gte=0"`; `json:"id,omitempty"`; `form:"id"`; `uri:"id"`

@mvdan
Copy link
Member

mvdan commented Jun 3, 2020

That is still a language change, since that syntax is not valid today.

@myitcv
Copy link
Member

myitcv commented Jun 5, 2020

Noting to add beyond @rsc and @mvdan's comments.

@rsc
Copy link
Contributor

rsc commented Jun 10, 2020

There's no change in the consensus since the last week. Declined.

@go101
Copy link

go101 commented Dec 25, 2020

An ugly workaround:

package main

import "fmt"
import "reflect"

type Expect struct {
	ID int32 `                                    _:"
	        " db:"id,type=INT,auto_increment,key" _:"
	        " binding:"gte=0"                     _:"
	        " json:"id,omitempty"                 _:"
	        " form:"id"                           _:"
	        " uri:"id"                            _:"
	        "`
}

func main() {
	t := reflect.TypeOf(Expect{})
	id := t.Field(0).Tag
	fmt.Println(id.Lookup("db"))      // id,type=INT,auto_increment,key true
	fmt.Println(id.Lookup("binding")) // gte=0 true
	fmt.Println(id.Lookup("json"))    // id,omitempty true
	fmt.Println(id.Lookup("form"))    // id true
	fmt.Println(id.Lookup("uri"))     // id true
}

Though it might be a bug.

@golang golang locked and limited conversation to collaborators Dec 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

8 participants