Copilot commented on code in PR #7812:
URL: https://github.com/apache/opendal/pull/7812#discussion_r3451976379
##########
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::*;
+
Review Comment:
`use pyo3::prelude::*;` inside the `#[pymodule] mod _opendal` block appears
unused (the module uses fully-qualified `pyo3::...` paths). This can trigger
unused-import warnings in CI/clippy; consider removing it (and check whether
the crate-level `use pyo3::prelude::*;` is still needed after this refactor).
##########
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:
`register_in_sys` changes module registration semantics (notably
`sys.modules` keys and `__name__`). Given the PR claims a behavioral correction
here, it would be good to add a small Python test that verifies `import
opendal.operator` / `import opendal.exceptions` works and that
`opendal.operator.__name__ == "opendal.operator"` (and similarly for others) to
prevent regressions.
##########
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:
`use pyo3::prelude::*;` in this init function appears unused and can cause
unused-import warnings under clippy/CI. It can be removed safely since all
referenced PyO3 items are already fully qualified or imported elsewhere.
--
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]
