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.

Sign up for GitHub

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

[chore][docs/coding-guidelines] Document approach to optional fields #12352

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[chore][docs/coding-guidelines] Document approach to optional fields #12352

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f2c6ac
[chore][docs/coding-guidelines] Document approach to optional fields
mx-psi Feb 11, 2025
258c11c
fix
mx-psi Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/coding-guidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,14 @@ To keep naming patterns consistent across the project, enumeration patterns are
- `pcommon.ValueTypeStr` for `pcommon.ValueType`
- `pmetric.MetricTypeGauge` for `pmetric.MetricType`

## Optional fields

When representing optional fields in the configuration, we distinguish two cases:
1. For scalars (e.g. `int`, `string`, `bool`, `time.Duration`...), the zero value of the base type
is used to represent the absence of the field. The default value for this field may be different
from the zero value, so, when using the Go API you MUST follow the guidance on the "Default
Configuration" section below.
Comment on lines +100 to +104
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When representing optional fields in the configuration, we distinguish two cases:
1. For scalars (e.g. `int`, `string`, `bool`, `time.Duration`...), the zero value of the base type
is used to represent the absence of the field. The default value for this field may be different
from the zero value, so, when using the Go API you MUST follow the guidance on the "Default
Configuration" section below.
When representing optional fields in the configuration for which the zero value indicates the absence of the field, we distinguish two cases:
1. For scalars (e.g. `int`, `string`, `bool`, `time.Duration`...), the zero value of the base type
should be used to represent the absence of the field instead of a pointer. The default value for this field may be different
from the zero value, so, when using the Go API you MUST follow the guidance on the "Default
Configuration" section below.

I think we should limit this to only applying to the case where the zero value is not a valid value for the field and can indicate absence of the field. For cases where this doesn't apply, I think we still need to evaluate whether to use pointers or an optional type.

2. For struct fields, you may use a pointer to struct to represent an optional sections.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should suggest just using non-pointer values for now so we can be consistent across structs.

Our stable config modules don't use pointers for any struct fields, so this would allow us to be consistent. Once we've arrived at a decision, we can later indicate that pointer values (or optional types) should be used for types where zero values don't indicate absence of the field.


## Recommended Libraries / Defaults

Expand Down
Loading