Copilot commented on code in PR #7181:
URL: https://github.com/apache/opendal/pull/7181#discussion_r2768587595
##########
bindings/python/python/opendal/__init__.py:
##########
@@ -18,25 +18,16 @@
# ruff: noqa: D104
import builtins
-from opendal._opendal import ( # noqa: F403
- capability,
- exceptions,
- file,
- layers,
- services,
- types,
-)
-from opendal.operator import AsyncOperator, Operator # pyright:ignore
-
-__version__: builtins.str
+from opendal._core import * # ty: ignore
Review Comment:
The typo comment `# ty: ignore` should be `# type: ignore` for proper
mypy/type checker compatibility.
```suggestion
from opendal._core import * # type: ignore
```
##########
bindings/python/src/operator.rs:
##########
@@ -626,13 +623,14 @@ impl Operator {
/// An iterator over the entries in the directory.
#[gen_stub(override_return_type(
type_repr="collections.abc.Iterable[opendal.types.Entry]",
- imports=("collections.abc", "opendal.types")
+ imports=("collections.abc", "opendal")
))]
#[pyo3(signature = (path, *,
limit=None,
start_after=None,
versions=None,
deleted=None))]
+ #[deprecated(note = "Use `list()` with `recursive=True` instead")]
Review Comment:
The `#[deprecated]` attribute is applied to the `scan` methods, but the
deprecation message in the Python stubs uses `@typing_extensions.deprecated()`.
While this provides the deprecation warning in type checkers, the actual
runtime deprecation warning will only be triggered from the Rust side. Consider
if a Python-level deprecation warning should also be added for better
visibility to users.
##########
bindings/python/pyproject.toml:
##########
@@ -46,9 +46,8 @@ benchmark = [
"greenlet>=3.1.1",
"pydantic>=2.10.6",
]
-dev = ["maturin>=1.8.2"]
+dev = ["maturin>=1.9.4,<2.0", "ruff>=0.9.10", "ty>=0.0.15"]
Review Comment:
The `lint` extra group has been removed, but its dependencies (`ruff`) have
been moved to the `dev` group. This is a breaking change for users who may have
been using `pip install opendal[lint]`. Consider documenting this change or
keeping both extras for backward compatibility.
##########
bindings/python/Cargo.toml:
##########
@@ -197,18 +197,18 @@ doc = false
name = "_opendal"
[dependencies]
-bytes = "1.5.0"
-futures = "0.3.28"
-jiff = { version = "0.2.15" }
+bytes = "1"
+futures = "0.3"
+jiff = "0.2"
mea = "0.6"
# this crate won't be published, we always use the local version
opendal = { version = ">=0", path = "../../core", features = [
"blocking",
"layers-mime-guess",
] }
-pyo3 = { version = "0.27.2", features = ["generate-import-lib", "jiff-02"] }
-pyo3-async-runtimes = { version = "0.27.0", features = ["tokio-runtime"] }
-pyo3-stub-gen = { version = "0.17" }
+pyo3 = { version = "0.27", features = ["generate-import-lib", "jiff-02"] }
+pyo3-async-runtimes = { version = "0.27", features = ["tokio-runtime"] }
+pyo3-stub-gen = { version = "0.18" }
Review Comment:
The dependency version for `pyo3-stub-gen` was updated to `0.18`, but this
might be incompatible with `pyo3 = "0.27"`. According to the pyo3-stub-gen
compatibility matrix, version 0.18 typically works with pyo3 0.22. For pyo3
0.27, a newer version of pyo3-stub-gen may be needed. Please verify
compatibility or pin to a compatible version.
```suggestion
pyo3-stub-gen = { version = "0.20" }
```
##########
bindings/python/pyproject.toml:
##########
@@ -57,10 +56,8 @@ test = [
]
[tool.maturin]
-features = ["pyo3/extension-module"]
module-name = "opendal._opendal"
python-source = "python"
Review Comment:
The `features` and `strip` fields have been removed from the maturin
configuration. While these might be intentional changes, removing `features =
["pyo3/extension-module"]` could cause issues with the extension module build.
The `pyo3/extension-module` feature is typically required for Python extension
modules. Consider whether these removals are intentional.
```suggestion
python-source = "python"
features = ["pyo3/extension-module"]
strip = true
```
##########
bindings/python/python/opendal/__init__.py:
##########
@@ -18,25 +18,16 @@
# ruff: noqa: D104
import builtins
-from opendal._opendal import ( # noqa: F403
- capability,
- exceptions,
- file,
- layers,
- services,
- types,
-)
-from opendal.operator import AsyncOperator, Operator # pyright:ignore
-
-__version__: builtins.str
+from opendal._core import * # ty: ignore
Review Comment:
The import statement `from opendal._core import *` references a module
`_core`, but the maturin configuration in `pyproject.toml` (line 59) specifies
`module-name = "opendal._opendal"`. This means the Rust extension will be
available as `_opendal`, not `_core`. Either the import should be `from
opendal._opendal import *` or the maturin module-name should be changed to
match.
```suggestion
from opendal._opendal import * # ty: ignore
```
##########
bindings/python/pyproject.toml:
##########
@@ -46,9 +46,8 @@ benchmark = [
"greenlet>=3.1.1",
"pydantic>=2.10.6",
]
-dev = ["maturin>=1.8.2"]
+dev = ["maturin>=1.9.4,<2.0", "ruff>=0.9.10", "ty>=0.0.15"]
Review Comment:
The dependency constraint `"maturin>=1.9.4,<2.0"` pins to a very specific
version range. Maturin 1.9.4 was released recently, and this constraint might
be too restrictive. Consider using a more flexible constraint like
`"maturin>=1.8"` unless there's a specific reason for requiring 1.9.4+.
```suggestion
dev = ["maturin>=1.8,<2.0", "ruff>=0.9.10", "ty>=0.0.15"]
```
--
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]
