structs: rename EnterpriseMeta constructor #9720
Conversation
dnephin
added
the
theme/internal-cleanup
|
🤔 Double check that this PR does not require a changelog entry in the .changelog directory. Reference |
| @@ -43,7 +43,7 @@ func TestStreamingHealthServices_EmptySnapshot(t *testing.T) { | |||
| req := &structs.ServiceSpecificRequest{ | |||
| Datacenter: "dc1", | |||
| ServiceName: "web", | |||
| EnterpriseMeta: structs.EnterpriseMetaInitializer(namespace), | |||
| EnterpriseMeta: structs.NewEnterpriseMeta(namespace), | |||
There was a problem hiding this comment.
Just curious but is this still idiomatic naming if the new value isn't heap allocated. The reason I didn't go with New* originally was so that a caller could know by the name alone that this isn't doing an allocation.
There was a problem hiding this comment.
That could just be my c/c++ background showing a little there.
There was a problem hiding this comment.
Good question! I have never heard of New functions only being used when a pointer or interface is returned.
I suspect that using func New for non-pointer returns is rare because generally structs in Go are either for data (in which case the zero value should be usable and there is no need for a constructor), or they are used for behaviour with pointer receiver methods (and a func New is necessary for creating one).
I think our use here is unique because normally we wouldn't need a func New, we only need it for enterprise differentiation.
I couldn't find any instances in the stdlib (likely because they would not use a constructor for this type). CodeReviewComments includes at least this one example: https://github.com/golang/go/wiki/CodeReviewComments#interfaces (the last example block in that section) showing a func New that returns a struct (not a pointer).
There was a problem hiding this comment.
My understanding is that the compiler could still decide to heap allocate this variable if the code takes the address to it later on. The programmer doesn't necessary get to decide if it is heap or stack allocated, so most likely naming things with that assumption won't work out well.
|
The build failures are fixed by #9722. I'll rebase once that is merged. |
To match the Go convention.
dnephin
force-pushed
the
dnephin/ent-meta-ergo-1
branch
from
d95924d to
d1772ae
Compare
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/328222. |
To match the Go convention.