-
Notifications
You must be signed in to change notification settings - Fork 212
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
302-add-set-override-option-combinator #303
302-add-set-override-option-combinator #303
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.
I think the change is simple enough to be merged, safe the two comments I had. Besides that 👍
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.
One little nitpick left, I would really appreciate if you could fix this 😄 🙏
Besides the one comment this looks good, feel free to squash (including the change for the comment from above)! |
7a80d34
to
1195795
Compare
I failed my squash miserably, can you squash for me on merge ? :) 🙇 |
1195795
to
da3e483
Compare
Not on merge, because squash merging is bad. I rebased for you and pushed to your PR branch. |
The CI failure comes from something else, which I fix in #304 - I wil rebase this PR for you after that other fix is merged! |
This patch adds ConfigBuilder::set_override_option() to override a setting from an Option<_>. Signed-off-by: Matthias Beyer <[email protected]>
da3e483
to
be7afe9
Compare
Based on discussion in #302.
@matthiasbeyer I checked
Environment
and I do not believe it is not designed to support loading single env vars. This is just an example of how the method could look like.