-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Make it easier to register configuration extension ... #9781
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
Conversation
... options closes apache#9529
| } | ||
|
|
||
| /// Insert new [ConfigExtension] | ||
| pub fn with_option_extension<T: ConfigExtension>(mut self, extension: T) -> Self { |
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.
we have discussed with_extension_options as the method name, but it make more sense to me to name it like this, not to be confused with with_extension
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.
Yeah I agree this is a reasonable name, even though it still a bit awkward
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'm horrible with naming things 😀 open to suggestions
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.
Yeah, if I had a better alternative I would give it.
alamb
left a comment
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.
Thanks @milenkovicm
| } | ||
|
|
||
| /// Insert new [ConfigExtension] | ||
| pub fn with_option_extension<T: ConfigExtension>(mut self, extension: T) -> Self { |
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.
Yeah I agree this is a reasonable name, even though it still a bit awkward
|
Thanks again @milenkovicm |
... options
closes #9529
Which issue does this PR close?
Closes #9529.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?