alamb commented on code in PR #405:
URL:
https://github.com/apache/arrow-rs-object-store/pull/405#discussion_r2151982831
##########
src/lib.rs:
##########
@@ -901,6 +894,30 @@ macro_rules! as_ref_impl {
as_ref_impl!(Arc<dyn ObjectStore>);
as_ref_impl!(Box<dyn ObjectStore>);
+/// Extension trait for [`ObjectStore`] with convinience functions.
Review Comment:
To ease the transition I suggest we augment the documentation here with the
following additional information
# 1. Explain the intended use / goal of this trait
As I understand it, the idea of this trait is to make it more clear what
functions *must* be provided by an `ObjectStore` implementation and which are
mostly implemented in terms of the others.
Maybe we can also make it clear to implementers that the default
implementations of `ObjectStoreExt` may not be optimal for their uscase
However, I may not understand the full subtely - perhaps we can add more
info from https://github.com/apache/arrow-rs-object-store/issues/385 as
appropriate
# 2. Add a note about the migration plan / tips to help on upgrade
For example, we could add a section on "upgrade notes" and explain "if you
implemented `put` for your `ObjectStore`, if it has an implementation different
than the default move that implementation to `ObjectStoreExt`.
Also it would be good to highlight any planned changes that were forthcoming
(like "we plan to move the X, Y and Z functions to ObjectStoreExt over time as
well")
# 3. An example
I know this sounds silly, but I think especially given `impl Future --> ...`
signature it is non obvious to the causal Rust programmer that this means the
impl should have an `async` fn
##########
src/lib.rs:
##########
@@ -901,6 +894,30 @@ macro_rules! as_ref_impl {
as_ref_impl!(Arc<dyn ObjectStore>);
as_ref_impl!(Box<dyn ObjectStore>);
+/// Extension trait for [`ObjectStore`] with convinience functions.
+pub trait ObjectStoreExt {
+ /// Save the provided bytes to the specified location
+ ///
+ /// The operation is guaranteed to be atomic, it will either successfully
+ /// write the entirety of `payload` to `location`, or fail. No clients
+ /// should be able to observe a partially written object
+ fn put(
+ &self,
+ location: &Path,
+ payload: PutPayload,
+ ) -> impl Future<Output = Result<PutResult>> + Send;
Review Comment:
If we changed this to return a `BoxFuture` would it avoid the need to change
MSRV?
Perhaps that is what you mean by avoids dyn dispatch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
