-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[PoC] Test alternative configoptional implementation #13060
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
base: main
Are you sure you want to change the base?
Conversation
9fbc1b8
to
115fb2c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13060 +/- ##
==========================================
+ Coverage 91.58% 91.60% +0.01%
==========================================
Files 505 506 +1
Lines 28319 28359 +40
==========================================
+ Hits 25937 25977 +40
Misses 1873 1873
Partials 509 509 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
115fb2c
to
c11701a
Compare
c11701a
to
3ef60da
Compare
This mostly works but I would need #13064 to avoid some quirks with expanded values |
// The zero value of Optional is None. | ||
type Optional[T any] struct { | ||
// Enabled indicates if the value is present. | ||
Enabled bool `mapstructure:"enabled"` |
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.
Left a response on the original PR #13044 (comment). IMO adding enabled
directly to the optional type is an abstraction leak, it does not need to be done at the framework level, the user always has the option of adding it explicitly to T
.
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Adds a method to delete a path from a `Conf` object. I needed this for #13060 --------- Co-authored-by: Jade Guiton <[email protected]>
Description
Implements alternative described here: #13044 (comment)