-
Notifications
You must be signed in to change notification settings - Fork 703
RFC-6213: Options API #6213
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
RFC-6213: Options API #6213
Conversation
Signed-off-by: Xuanwo <[email protected]>
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.
Pull Request Overview
This PR proposes an RFC to introduce an options-based API for OpenDAL operations to improve code readability and maintainability.
- Introduces a new RFC document outlining the options API design.
- Provides code examples contrasting the traditional builder pattern with the proposed structured options approach.
Comments suppressed due to low confidence (2)
core/src/docs/rfcs/0000_options_api.md:91
- The 'version' field is used in the ReadOptions initialization but is not declared in the ReadOptions struct. Consider either adding the 'version' field to the struct or removing its usage from this example.
version: Some(version_id),
core/src/docs/rfcs/0000_options_api.md:92
- The code uses ReadOptions::default(), which implies that a default implementation exists for ReadOptions; however, such an implementation is not shown in the struct definition. Please include a default implementation or adjust the example accordingly.
...ReadOptions::default(),
Signed-off-by: Xuanwo <[email protected]>
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 like this a lot!
yihong0618
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.
LGTM
Which issue does this PR close?
Closes #5727
Rationale for this change
Propose a new API to make our users lives easier.
What changes are included in this PR?
A new RFC.
Are there any user-facing changes?
None