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

reflect: back out multiple keys in key:value pair in struct tag #40281

Closed
scorsi opened this issue Jul 18, 2020 · 67 comments
Closed

reflect: back out multiple keys in key:value pair in struct tag #40281

scorsi opened this issue Jul 18, 2020 · 67 comments

Comments

@scorsi
Copy link

scorsi commented Jul 18, 2020

Hello 👋

I'm actually boring to set the same value for multiple key in my struct tag. Maybe I missed something but here is the boring thing:

type MyStruct struct {
  Field1 string `json:"field_1,omitempty" bson:"field_1,omitempty" xml:"field_1,omitempty" form:"field_1,omitempty" other:"value"`
}

What I'm proposing is:

type MyStruct struct {
  Field1 string `json,bson,xml,form:"field_1,omitempty" other:"value"`
}

Thanks 👍

@gopherbot gopherbot added this to the Proposal milestone Jul 18, 2020
@ianlancetaylor ianlancetaylor changed the title Proposal: multiple key for same value in struct tag proposal: reflect: multiple key for same value in struct tag Jul 19, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jul 19, 2020
@rsc rsc moved this from Incoming to Active in Proposals (old) Jul 22, 2020
@rsc rsc changed the title proposal: reflect: multiple key for same value in struct tag proposal: reflect: allow multiple keys in key:value pair in struct tag Jul 22, 2020
@ianlancetaylor
Copy link
Contributor

Just noting that this would mean that struct tag keys cannot contain a comma character. The current documentation at https://golang.org/pkg/reflect/#StructTag is "Each key is a non-empty string consisting of non-control characters other than space (U+0020 ' '), quote (U+0022 '"'), and colon (U+003A ':')." So this would be a change. Of course, it's fairly unlikely that anybody is using a tag key with a comma.

@scorsi
Copy link
Author

scorsi commented Jul 22, 2020

It could be a breaking change of course.

@rsc
Copy link
Contributor

rsc commented Jul 29, 2020

I have a large corpus of Go code that I can scan at some point to see if anyone uses , in tag names in practice.
Will try to do that before next week.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2020

What if we use a space-separated list instead of a comma-separated one?

type MyStruct struct {
  Field1 string `json bson xml form:"field_1,omitempty" other:"value"`
}

The spaces form is more analogous to what happens in Go argument lists:
in x int, y int, z int, you can remove the repeated int, leaving x, y, z int.
If we do the same here, then
in json:"x" bson:"x" xml:"x", you can remove the repeated :"x", leaving json bson xml:"x".
If we inserted commas during this step, you'd need to explain where they came from
(they weren't there before!).

The space form is also quite a bit less of a wall of text and thus easier to skim. Compare:

Field1 string `json bson xml form:"field_1,omitempty" other:"value"`
Field1 string `json,bson,xml,form:"field_1,omitempty" other:"value"`

And unlike comma, spaces cannot appear in valid tags today,
so we would not be redefining technically valid syntax to mean
something slightly different.

@cristaloleg
Copy link

Space-separated is much better and it indeed looks like Go argument list.

Regarding proposal: I haven't use more than 1 file tag (json or yaml) before, but last week have played with different config files and leaving all this verbose repetitions (field1 for JSON, also for YAML, also for TOML, also for ...) will make multi-tags much easier to use.

@scorsi
Copy link
Author

scorsi commented Jul 30, 2020

I must admit that space separated could be a good idea which won’t break anything.

Personally I feel it less comprehensible than comma separated and in go we have comma between arguments (we don’t write a b c int so comma is more go in fact lol ok i did understand why space is more go in fact) but a choice have to be made and don’t belongs to me :)

Even with space separated, the main issue is resolved so it’s ok to me ! ;)

@rsc
Copy link
Contributor

rsc commented Aug 5, 2020

OK, so with the syntax I suggested last Thursday (space-separated instead of comma-separated keys), there is no concern about backwards compatibility. There hasn't been much discussion otherwise.

I did make some progress analyzing tags in my Go corpus last week, and I found that 0.06% of tags have commas in keys (outside quoted values). All of those are mistakes. So we could plausibly use comma and not break things, but the space is more in keeping with the current syntax anyway.

Based on the discussion above, this seems like a likely accept (with spaces, not commas).

To be clear, the new syntax would be:

type MyStruct struct {
  Field1 string `json bson xml form:"field_1,omitempty" other:"value"`
}

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Aug 5, 2020
@rsc
Copy link
Contributor

rsc commented Aug 12, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Aug 12, 2020
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Aug 12, 2020
@bynov
Copy link
Contributor

bynov commented Aug 13, 2020

Hi! I would like to work on that feature. Is it possible? :)

@rsc
Copy link
Contributor

rsc commented Aug 13, 2020

Sure, go ahead.

@gopherbot
Copy link

Change https://golang.org/cl/248341 mentions this issue: reflect: added multiple keys support for struct tags

@rsc rsc modified the milestones: Proposal, Backlog Aug 14, 2020
@rsc rsc changed the title proposal: reflect: allow multiple keys in key:value pair in struct tag reflect: allow multiple keys in key:value pair in struct tag Aug 14, 2020
@rsc rsc added this to the Backlog milestone Jan 6, 2021
@ianlancetaylor ianlancetaylor modified the milestones: Backlog, Go1.16 Jan 6, 2021
@gopherbot
Copy link

Change https://golang.org/cl/281237 mentions this issue: doc/go1.16: remove note about reverted reflect change

@gopherbot
Copy link

Change https://golang.org/cl/281973 mentions this issue: Revert "go/analysis/passes/structtag: recognize multiple keys per tag"

@gopherbot
Copy link

Change https://golang.org/cl/281515 mentions this issue: Revert "reflect: support multiple keys in struct tags"

gopherbot pushed a commit to golang/tools that referenced this issue Jan 7, 2021
This reverts CL 277092.

Proposal golang/go#40281 was initially accepted, then declined.  Stop accepting
the new, now once again invalid, format.

For golang/go#40281
For golang/go#43083
For golang/go#43226

Change-Id: Ida97263048f3a048f904a844f577d8353e3a1afa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/281973
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/282412 mentions this issue: cmd: update to latest golang.org/x/tools

gopherbot pushed a commit that referenced this issue Jan 7, 2021
Proposal #40281 was initially accepted, but has now been declined.
This CL removes most of the work done to implement it.

Specifically this reverts CLs 248341, 274448, 274474, and 278392.

For #40281
For #43226

Change-Id: I5a9ebb4d9cb5fb0962434b64c59beb8343030be5
Reviewed-on: https://go-review.googlesource.com/c/go/+/281515
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
@ianlancetaylor
Copy link
Contributor

Everything is backed out now.

gopherbot pushed a commit that referenced this issue Jan 8, 2021
In particular bring in CL 201973, which reverts support for multiple
keys in a struct tag.

For #40281
For #43083
For #43226

Change-Id: I66e76639cbbca55bdbff6956acdb0a97650fdd31
Reviewed-on: https://go-review.googlesource.com/c/go/+/282412
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
gopherbot pushed a commit to golang/tools that referenced this issue Jan 11, 2021
This reverts CL 277092.

Proposal golang/go#40281 was initially accepted, then declined.  Stop accepting
the new, now once again invalid, format.

For golang/go#40281
For golang/go#43083
For golang/go#43226

Change-Id: Ida97263048f3a048f904a844f577d8353e3a1afa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/281973
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@codenoid

This comment has been minimized.

@mvdan

This comment has been minimized.

@codenoid

This comment has been minimized.

@mvdan

This comment has been minimized.

@codenoid

This comment has been minimized.

@rsc
Copy link
Contributor

rsc commented May 7, 2021

If you can be 5X more productive with this feature then it means you spend at least 80% of your time writing struct tags.
I'm sorry to hear that and encourage you to write a program to do that for you.

@azhai
Copy link

azhai commented Oct 12, 2021

What about this?

type MyStruct struct {
  Field1 string `json/bson/xml/form:"field_1,omitempty" other:"value"`
}

@scorsi
Copy link
Author

scorsi commented Oct 12, 2021

The issue discussed here is the same... It's not a problem from the selected char but the splitting itself. So, using / won't resolve it. I'm the first impacted from the rejection of that feature but.. yeah, it's life dude. Let that feature dead :)

@azhai
Copy link

azhai commented Oct 13, 2021

go-sftag Yet another StructTag

type ConnParams struct {
	Host     string                 `json:"host" yaml:"host" toml:"host"`
	Port     int                    `json:"port,omitempty" yaml:"port,omitempty" toml:"port"`
	Username string                 `yaml/json:"username,omitempty" toml:"username"`
	Password string                 `toml/yaml/json:"password"`
	Database string                 `toml/yaml/json:"database"`
	Options  map[string]interface{} `yaml/json:"options,omitempty" toml:"options"`
}

@scorsi
Copy link
Author

scorsi commented Oct 13, 2021

The idea is good but how to make it work with json.Marshal for exemple ? Because it uses the internal tag library. :)

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