-
Notifications
You must be signed in to change notification settings - Fork 481
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
feat(core)!: Make config data object #4915
Conversation
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
d540734
to
580bd5d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
type Config: DeserializeOwned; | ||
|
||
/// Construct a builder from given config. | ||
fn from_config(config: Self::Config) -> 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.
I want to return Result<Self>
in from_config
and from_map
to avoid panic during the build process. Would you like to help implementing it in this PR or start a new PR for this? Or I can continue the work after this PR.
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.
Let's make further non-blocker change in a follow-up PR. I can do it, but not in this one unless it's necessary.
@Xuanwo Actually this is not a breaking change .. What parts are breaking? |
Builder is a public trait. All users implement this API will fail to compile. Adding a new associated type and required function are breaking changes. |
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 a lot for finishing this!
Which issue does this PR close?
This closes #3240.
What changes are included in this PR?
See commits message.
Are there any user-facing changes?
All builder has an associated config data object type now.