chitralverma commented on code in PR #7812:
URL: https://github.com/apache/opendal/pull/7812#discussion_r3452187968


##########
bindings/python/src/lib.rs:
##########
@@ -43,72 +43,120 @@ use pyo3_stub_gen::{define_stub_info_gatherer, derive::*};
 pub use services::*;
 
 #[pymodule(gil_used = false)]
-fn _opendal(py: Python, m: &Bound<'_, PyModule>) -> PyResult<()> {
-    // Add version
-    m.add("__version__", env!("CARGO_PKG_VERSION"))?;
-
-    // Operator module
-    add_pymodule!(py, m, "operator", [Operator, AsyncOperator])?;
-
-    // File module
-    add_pymodule!(py, m, "file", [File, AsyncFile])?;
-
-    // Capability module
-    add_pymodule!(py, m, "capability", [Capability])?;
-
-    // Services module
-    add_pymodule!(py, m, "services", [PyScheme])?;
-
-    // Layers module
-    add_pymodule!(
-        py,
-        m,
-        "layers",
-        [
-            Layer,
-            CapabilityOverrideLayer,
-            RetryLayer,
-            ConcurrentLimitLayer,
-            MimeGuessLayer
-        ]
-    )?;
-
-    // Types module
-    add_pymodule!(
-        py,
-        m,
-        "types",
-        [Entry, EntryMode, Metadata, PresignedRequest]
-    )?;
-
-    m.add_class::<WriteOptions>()?;
-    m.add_class::<ReadOptions>()?;
-    m.add_class::<ListOptions>()?;
-    m.add_class::<StatOptions>()?;
-    m.add_class::<DeleteOptions>()?;
-
-    // Exceptions module
-    add_pyexceptions!(
-        py,
-        m,
-        "exceptions",
-        [
-            Error,
-            Unexpected,
-            Unsupported,
-            ConfigInvalid,
-            NotFound,
-            PermissionDenied,
-            IsADirectory,
-            NotADirectory,
-            AlreadyExists,
-            IsSameFile,
-            ConditionNotMatch,
-            RateLimited,
-            RangeNotSatisfied,
-        ]
-    )?;
-    Ok(())
+mod _opendal {
+    use pyo3::prelude::*;
+
+    #[pymodule]
+    mod operator {
+        #[pymodule_export]
+        use crate::{AsyncOperator, Operator};
+
+        #[pymodule_init]
+        fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> 
pyo3::PyResult<()> {
+            crate::register_in_sys(m, "operator")
+        }
+    }
+
+    #[pymodule]
+    mod file {
+        #[pymodule_export]
+        use crate::{AsyncFile, File};
+
+        #[pymodule_init]
+        fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> 
pyo3::PyResult<()> {
+            crate::register_in_sys(m, "file")
+        }
+    }
+
+    #[pymodule]
+    mod capability {
+        #[pymodule_export]
+        use crate::Capability;
+
+        #[pymodule_init]
+        fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> 
pyo3::PyResult<()> {
+            crate::register_in_sys(m, "capability")
+        }
+    }
+
+    #[pymodule]
+    mod services {
+        #[pymodule_export]
+        use crate::PyScheme;
+
+        #[pymodule_init]
+        fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> 
pyo3::PyResult<()> {
+            crate::register_in_sys(m, "services")
+        }
+    }
+
+    #[pymodule]
+    mod layers {
+        #[pymodule_export]
+        use crate::{
+            CapabilityOverrideLayer, ConcurrentLimitLayer, Layer, 
MimeGuessLayer, RetryLayer,
+        };
+
+        #[pymodule_init]
+        fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> 
pyo3::PyResult<()> {
+            crate::register_in_sys(m, "layers")
+        }
+    }
+
+    #[pymodule]
+    mod types {
+        #[pymodule_export]
+        use crate::{Entry, EntryMode, Metadata, PresignedRequest};
+
+        #[pymodule_init]
+        fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> 
pyo3::PyResult<()> {
+            crate::register_in_sys(m, "types")
+        }
+    }
+
+    // Exceptions are `PyErr` subtypes, not `#[pyclass]`es, so they are added
+    // procedurally rather than via `#[pymodule_export]`.
+    #[pymodule]
+    mod exceptions {
+        #[pymodule_init]
+        fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> 
pyo3::PyResult<()> {
+            use pyo3::prelude::*;
+

Review Comment:
   Not unused — `add_exceptions!` calls `m.add(...)`, which needs the 
`PyModuleMethods` trait from the prelude. Removing it fails the build; clippy 
`-D warnings` is clean.



##########
bindings/python/src/utils.rs:
##########
@@ -77,44 +77,30 @@ impl Buffer {
     }
 }
 
-/// Macro to create and register a PyO3 submodule with multiple classes.
-///
-/// Example:
-/// ```rust
-/// add_pymodule!(py, m, "services", [PyScheme, PyOtherClass]);
-/// ```
-#[macro_export]
-macro_rules! add_pymodule {
-    ($py:expr, $parent:expr, $name:expr, [$($cls:ty),* $(,)?]) => {{
-        let sub_module = pyo3::types::PyModule::new($py, $name)?;
-        $(
-            sub_module.add_class::<$cls>()?;
-        )*
-        $parent.add_submodule(&sub_module)?;
-        $py.import("sys")?
-            .getattr("modules")?
-            .set_item(format!("opendal.{}", $name), &sub_module)?;
-        Ok::<_, pyo3::PyErr>(())
-    }};
+/// Register a submodule in `sys.modules` as `opendal.{name}` and set its
+/// `__name__` to match, so dotted imports resolve and the module reports its
+/// qualified name rather than PyO3's default `_opendal.{name}`.
+pub fn register_in_sys(module: &Bound<'_, PyModule>, name: &str) -> 
PyResult<()> {
+    let qualified_name = format!("opendal.{name}");
+    module.setattr("__name__", &qualified_name)?;

Review Comment:
   Manually validated: dotted imports work and `opendal.<name>.__name__ == 
"opendal.<name>"` for all submodules (incl. exceptions).



-- 
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]

Reply via email to