📖 API conventions: add note about when struct fields should be pointer #5782
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
CONTRIBUTING.md
Outdated
| * Fields SHOULD be pointers if there is a good reason for it, e.g: | ||
| * the nil and the zero values (by Go standards) have semantic differences. | ||
| * Note: This doesn't apply to map or slice types as they are assignable to `nil`. | ||
| * the field is of a struct type and would show up as an empty object after marshalling (e.g. `kubectl get`) |
There was a problem hiding this comment.
What about
| * the field is of a struct type and would show up as an empty object after marshalling (e.g. `kubectl get`) | |
| * the field is of a struct type containing all omitempty values, and you want to prevent it to show up as an empty object after marshalling (e.g. `kubectl get`) |
There was a problem hiding this comment.
So the empty object wouldn't show up if one of the fields doesn't have omitempty? (as that field would then be always rendered and thus the object wouldn't be empty).
I'm not 100% sure if we should couple the decision to the current state of the fields as the fields can change over time but changing something to a pointer is a breaking change. (which I think requires deprecating the whole field)
Probably there is no future proof way to do this.
There was a problem hiding this comment.
This kinda comes down to generic kube recommendation, if it's optional make it a pointer.
There was a problem hiding this comment.
An optional struct might have required fields but still be a pointer though?
There was a problem hiding this comment.
This kinda comes down to generic kube recommendation, if it's optional make it a pointer.
I would only consider this for structs though, for basic types it's imho too painful to handle and has no upside if you don't care about the difference between unset and the default value.
An optional struct might have required fields but still be a pointer though?
Yup exactly. I think that would be fine too.
There was a problem hiding this comment.
Btw, it's a nuance, but I intentionally wrote e.g.. So the general idea is, hey please let's only make these a pointer if there is a reason for it (aka we know what we're doing). It's not meant as an exhaustive list.
There was a problem hiding this comment.
Thanks Stefan, let's incorporate Fabrizio suggestion and proceed.
There was a problem hiding this comment.
@vincepri fine for you too? Given that it's "just" an example
There was a problem hiding this comment.
Rephrased accordingly. @fabriziopandini @enxebre @vincepri PTAL :)
Signed-off-by: Stefan Büringer [email protected]
sbueringer
force-pushed
the
pr-api-conventions-struct-pointer
branch
from
91f64eb to
48099aa
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Signed-off-by: Stefan Büringer [email protected]
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #