messense commented on code in PR #3464:
URL:
https://github.com/apache/incubator-opendal/pull/3464#discussion_r1379664705
##########
bindings/python/src/layers.rs:
##########
@@ -18,47 +18,26 @@
use std::time::Duration;
use ::opendal as od;
+use opendal::Operator;
use pyo3::prelude::*;
-#[derive(FromPyObject)]
-pub enum Layer {
- ConcurrentLimit(ConcurrentLimitLayer),
- ImmutableIndex(ImmutableIndexLayer),
- Retry(RetryLayer),
+pub trait PythonLayer: Send + Sync {
+ fn layer(&self, op: Operator) -> Operator;
}
-#[pyclass(module = "opendal.layers")]
-#[derive(Clone)]
-pub struct ConcurrentLimitLayer(pub od::layers::ConcurrentLimitLayer);
+#[pyclass(module = "opendal.layers", subclass)]
+pub struct Layer(pub Box<dyn PythonLayer>);
Review Comment:
I can't see the point of putting a `Box<dyn PythonLayer>` here, could you
elaborate?
##########
bindings/python/src/layers.rs:
##########
@@ -75,7 +54,7 @@ impl RetryLayer {
jitter: bool,
max_delay: Option<f64>,
min_delay: Option<f64>,
- ) -> PyResult<Self> {
+ ) -> PyResult<(Self, Layer)> {
Review Comment:
I'd prefer to use
[PyClassInitializer](https://docs.rs/pyo3/latest/pyo3/pyclass_init/struct.PyClassInitializer.html)
instead of returning tuple because the intention is clearer.
--
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]
