Copilot commented on code in PR #7094:
URL: https://github.com/apache/opendal/pull/7094#discussion_r2645969430
##########
core/layers/dtrace/src/lib.rs:
##########
@@ -72,17 +76,17 @@ use opendal_core::*;
///
/// Example:
///
-/// ```ignore
-/// # use opendal::layers::DtraceLayer;
-/// # use opendal::services;
-/// # use opendal::Operator;
-/// # use opendal::Result;
-///
+/// ```no_run
+/// # use opendal_core::services;
+/// # use opendal_core::Operator;
+/// # use opendal_core::Result;
+/// # use opendal_layer_dtrace::DtraceLayer;
+/// #
/// # #[tokio::main]
/// # async fn main() -> Result<()> {
/// // `Accessor` provides the low level APIs, we will use `Operator` normally.
-/// let op: Operator = Operator::new(services::Fs::default().root("/tmp"))?
-/// .layer(DtraceLayer::default())
+/// let op: Operator = Operator::new(services::Memory::default().root("/tmp"))?
Review Comment:
The code example on line 88 references
`services::Memory::default().root("/tmp")`, but the Memory service doesn't
support the root() method. This will fail to compile. Consider using a service
that actually supports root configuration, such as `services::Fs`, or remove
the `.root("/tmp")` call.
```suggestion
/// let op: Operator = Operator::new(services::Fs::default().root("/tmp"))?
```
##########
core/layers/immutable-index/src/lib.rs:
##########
@@ -87,7 +98,8 @@ impl<A: Access> Layer<A> for ImmutableIndexLayer {
}
}
-#[derive(Debug, Clone)]
+#[doc(hidden)]
+#[derive(Debug)]
Review Comment:
Inconsistent removal of Clone trait. The ImmutableIndexAccessor previously
derived Clone on line 101, but now only derives Debug. This could be a breaking
change if users relied on cloning this type.
```suggestion
#[derive(Clone, Debug)]
```
##########
core/layers/async-backtrace/src/lib.rs:
##########
@@ -29,30 +33,39 @@ use opendal_core::*;
/// # Examples
///
/// ```no_run
-/// use opendal_layer_async_backtrace::AsyncBacktraceLayer;
-/// use opendal_core::services;
-/// use opendal_core::Operator;
-/// use opendal_core::Result;
-///
+/// # use opendal_core::services;
+/// # use opendal_core::Operator;
+/// # use opendal_core::Result;
+/// # use opendal_layer_async_backtrace::AsyncBacktraceLayer;
+/// #
/// # fn main() -> Result<()> {
/// let _ = Operator::new(services::Memory::default())?
-/// .layer(AsyncBacktraceLayer::default())
+/// .layer(AsyncBacktraceLayer::new())
/// .finish();
-/// Ok(())
+/// # Ok(())
/// # }
/// ```
#[derive(Clone, Default)]
-pub struct AsyncBacktraceLayer;
+#[non_exhaustive]
+pub struct AsyncBacktraceLayer {}
+
+impl AsyncBacktraceLayer {
+ /// Create a new [`AsyncBacktraceLayer`].
+ pub fn new() -> Self {
+ Self::default()
+ }
+}
impl<A: Access> Layer<A> for AsyncBacktraceLayer {
type LayeredAccess = AsyncBacktraceAccessor<A>;
- fn layer(&self, accessor: A) -> Self::LayeredAccess {
- AsyncBacktraceAccessor { inner: accessor }
+ fn layer(&self, inner: A) -> Self::LayeredAccess {
+ AsyncBacktraceAccessor { inner }
}
}
-#[derive(Debug, Clone)]
+#[doc(hidden)]
+#[derive(Debug)]
Review Comment:
Inconsistent removal of Clone trait. The AsyncBacktraceAccessor previously
derived Clone on line 67, but now only derives Debug. This could be a breaking
change if users relied on cloning this type.
```suggestion
#[derive(Clone, Debug)]
```
##########
core/layers/timeout/src/lib.rs:
##########
@@ -167,7 +172,8 @@ impl<A: Access> Layer<A> for TimeoutLayer {
}
}
-#[derive(Debug, Clone)]
+#[doc(hidden)]
+#[derive(Debug)]
pub struct TimeoutAccessor<A: Access> {
Review Comment:
Inconsistent removal of Clone trait. The TimeoutAccessor previously derived
Clone on line 175, but now only derives Debug. This differs from other layer
accessors in this PR which maintain Clone. If this is intentional due to
internal fields not being Clone, consider documenting why, otherwise this may
be an unintended breaking change for users who may have relied on cloning this
type.
##########
core/layers/logging/src/lib.rs:
##########
@@ -229,7 +234,8 @@ impl Display for LoggingContext<'_> {
}
}
-#[derive(Clone, Debug)]
+#[doc(hidden)]
+#[derive(Debug)]
Review Comment:
Inconsistent removal of Clone trait. The LoggingAccessor previously derived
Clone on line 237, but now only derives Debug. This could be a breaking change
if users relied on cloning this type.
```suggestion
#[derive(Debug, Clone)]
```
##########
core/layers/await-tree/src/lib.rs:
##########
@@ -32,37 +36,39 @@ use opendal_core::*;
/// # Examples
///
/// ```no_run
-/// # use opendal_layer_await_tree::AwaitTreeLayer;
/// # use opendal_core::services;
/// # use opendal_core::Operator;
/// # use opendal_core::Result;
-///
+/// # use opendal_layer_await_tree::AwaitTreeLayer;
+/// #
/// # fn main() -> Result<()> {
/// let _ = Operator::new(services::Memory::default())?
/// .layer(AwaitTreeLayer::new())
/// .finish();
-/// Ok(())
+/// # Ok(())
/// # }
/// ```
#[derive(Clone, Default)]
+#[non_exhaustive]
pub struct AwaitTreeLayer {}
impl AwaitTreeLayer {
- /// Create a new `AwaitTreeLayer`.
+ /// Create a new [`AwaitTreeLayer`].
pub fn new() -> Self {
- Self {}
+ Self::default()
}
}
impl<A: Access> Layer<A> for AwaitTreeLayer {
type LayeredAccess = AwaitTreeAccessor<A>;
- fn layer(&self, accessor: A) -> Self::LayeredAccess {
- AwaitTreeAccessor { inner: accessor }
+ fn layer(&self, inner: A) -> Self::LayeredAccess {
+ AwaitTreeAccessor { inner }
}
}
-#[derive(Debug, Clone)]
+#[doc(hidden)]
+#[derive(Debug)]
Review Comment:
Inconsistent removal of Clone trait. The AwaitTreeAccessor previously
derived Clone on line 70, but now only derives Debug. This could be a breaking
change if users relied on cloning this type.
```suggestion
#[derive(Clone, Debug)]
```
##########
core/layers/oteltrace/src/lib.rs:
##########
@@ -43,12 +48,21 @@ use opentelemetry::trace::Tracer;
/// #
/// # fn main() -> Result<()> {
/// let _ = Operator::new(services::Memory::default())?
-/// .layer(OtelTraceLayer)
+/// .layer(OtelTraceLayer::new())
/// .finish();
/// # Ok(())
/// # }
/// ```
-pub struct OtelTraceLayer;
+#[derive(Clone, Default)]
+#[non_exhaustive]
+pub struct OtelTraceLayer {}
Review Comment:
Inconsistent order of derive attributes. Most other files in this PR use
`Clone, Default` but this file uses `Default` before `Clone` on lines 56-58.
For consistency with the rest of the PR, consider reordering to `Clone,
Default`.
##########
core/layers/mime-guess/src/lib.rs:
##########
@@ -69,7 +81,8 @@ impl<A: Access> Layer<A> for MimeGuessLayer {
}
}
-#[derive(Clone, Debug)]
+#[doc(hidden)]
+#[derive(Debug)]
Review Comment:
Inconsistent removal of Clone trait. The MimeGuessAccessor previously
derived Clone on line 84, but now only derives Debug. Since these accessors are
marked as doc(hidden), this could still be a breaking change for any internal
or advanced users. Consider verifying this is intentional.
```suggestion
#[derive(Clone, Debug)]
```
##########
core/layers/concurrent-limit/src/lib.rs:
##########
@@ -170,7 +173,8 @@ where
}
}
-#[derive(Debug, Clone)]
+#[doc(hidden)]
+#[derive(Debug)]
Review Comment:
Inconsistent removal of Clone trait. The ConcurrentLimitAccessor previously
derived Clone on line 176, but now only derives Debug. This could be a breaking
change if users relied on cloning this type.
```suggestion
#[derive(Debug, Clone)]
```
##########
core/layers/dtrace/src/lib.rs:
##########
@@ -122,17 +126,24 @@ use opendal_core::*;
/// Arguments: -8@%rax
/// stapsdt 0x0000003c NT_STAPSDT (SystemTap probe
descriptors)
/// ```
-#[derive(Default, Debug, Clone)]
+#[derive(Clone, Default)]
+#[non_exhaustive]
pub struct DtraceLayer {}
+impl DtraceLayer {
Review Comment:
Missing documentation comment for this public method. The `missing_docs`
lint is enabled at the crate level, so all public items should have
documentation.
```suggestion
impl DtraceLayer {
/// Creates a new `DtraceLayer`.
```
##########
core/layers/throttle/src/lib.rs:
##########
@@ -102,7 +106,8 @@ impl<A: Access> Layer<A> for ThrottleLayer {
/// Read more about
[Middleware](https://docs.rs/governor/latest/governor/middleware/index.html)
type SharedRateLimiter = Arc<RateLimiter<NotKeyed, InMemoryState,
DefaultClock, NoOpMiddleware>>;
-#[derive(Debug, Clone)]
+#[doc(hidden)]
+#[derive(Debug)]
pub struct ThrottleAccessor<A: Access> {
Review Comment:
Inconsistent removal of Clone trait. The ThrottleAccessor previously derived
Clone on line 109, but now only derives Debug. This differs from the pattern
where most accessors lose Clone. However, this could be a breaking change if
external code relied on cloning this accessor. Consider verifying if this
change is intentional.
--
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]
