-
Notifications
You must be signed in to change notification settings - Fork 873
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
Add section to coding guidelines on Optional usage #6894
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with a couple suggestions.
It is ok to use `Optional` in places where it does not leak into public API signatures and where | ||
performance is not critical. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok to use `Optional` in places where it does not leak into public API signatures and where | |
performance is not critical. | |
Use `Optional` where it does not leak into public API signatures and where | |
performance is not a critical aspect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe It is acceptable to use
? we don't exactly want to "recommend" it, only mention that it is "ok" (i.e. author's choice to use or not)
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
This was discussed a long while back, but never documented (and recently came up in open-telemetry#6889) Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
This was discussed a long while back, but never documented (and recently came up in open-telemetry#6889) Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
This was discussed a long while back, but never documented (and recently came up in #6889)