You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Zheaoli (via GitHub)" <gi...@apache.org> on 2023/11/06 13:01:16 UTC
[PR] reactor(binding/python): Add multiple custom exception for each of error code in Rust Core [incubator-opendal]
Zheaoli opened a new pull request, #3492:
URL: https://github.com/apache/incubator-opendal/pull/3492
Signed-off-by: Manjusaka <me...@manjusaka.me>
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(binding/python): Add multiple custom exception for each of error code in Rust Core [incubator-opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on code in PR #3492:
URL: https://github.com/apache/incubator-opendal/pull/3492#discussion_r1383308896
##########
bindings/python/src/errors.rs:
##########
Review Comment:
OK, no problems
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(binding/python): Add multiple custom exception for each of error code in Rust Core [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3492:
URL: https://github.com/apache/incubator-opendal/pull/3492#discussion_r1384422751
##########
bindings/python/tests/test_sync_copy.py:
##########
@@ -16,93 +16,97 @@
# under the License.
import os
-from random import randint
from uuid import uuid4
import pytest
-from opendal.exceptions import IsADirectory, IsSameFile, NotFound
+from opendal.exceptions import Error, IsADirectory, IsSameFile, NotFound
-@pytest.mark.need_capability("read", "write", "copy")
-def test_sync_copy(service_name, operator, async_operator):
+@pytest.mark.need_capability("read", "write", "rename")
+def test_sync_rename_file(service_name, operator, async_operator):
source_path = f"random_file_{str(uuid4())}"
content = os.urandom(1024)
operator.write(source_path, content)
target_path = f"random_file_{str(uuid4())}"
- operator.copy(source_path, target_path)
- read_content = operator.read(target_path)
- assert read_content is not None
- assert read_content == content
- operator.delete(source_path)
+ operator.rename(source_path, target_path)
+ with pytest.raises(Error) as e_info:
+ operator.read(source_path)
+ assert isinstance(e_info.value, NotFound)
+ assert operator.read(target_path) == content
operator.delete(target_path)
+ operator.delete(source_path)
-@pytest.mark.need_capability("read", "write", "copy")
-def test_sync_copy_non_exist(service_name, operator, async_operator):
+@pytest.mark.need_capability("read", "write", "rename")
+def test_sync_rename_non_exists_file(service_name, operator, async_operator):
source_path = f"random_file_{str(uuid4())}"
target_path = f"random_file_{str(uuid4())}"
- with pytest.raises(NotFound) as e_info:
- operator.copy(source_path, target_path)
+ with pytest.raises(Error) as e_info:
+ operator.rename(source_path, target_path)
+ assert isinstance(e_info.value, NotFound)
-@pytest.mark.need_capability("read", "write", "copy")
-def test_sync_copy_source_directory(service_name, operator, async_operator):
+@pytest.mark.need_capability("read", "write", "rename")
+def test_sync_rename_directory(service_name, operator, async_operator):
source_path = f"random_file_{str(uuid4())}/"
operator.create_dir(source_path)
target_path = f"random_file_{str(uuid4())}"
- with pytest.raises(IsADirectory) as e_info:
- operator.copy(source_path, target_path)
+ with pytest.raises(Error) as e_info:
+ operator.rename(source_path, target_path)
+ assert isinstance(e_info.value, IsADirectory)
-@pytest.mark.need_capability("read", "write", "copy")
-def test_sync_copy_target_directory(service_name, operator, async_operator):
+@pytest.mark.need_capability("read", "write", "rename")
+def test_sync_rename_file_to_directory(service_name, operator, async_operator):
source_path = f"random_file_{str(uuid4())}"
content = os.urandom(1024)
operator.write(source_path, content)
target_path = f"random_file_{str(uuid4())}/"
- operator.create_dir(target_path)
- with pytest.raises(IsADirectory) as e_info:
- operator.copy(source_path, target_path)
+ with pytest.raises(Error) as e_info:
Review Comment:
How about adding a unittest instead? Our behavior tests code looks good and don't need to update. Behavioral test code typically reflects how users interact with our API.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(binding/python): Add multiple custom exception for each of error code in Rust Core [incubator-opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on code in PR #3492:
URL: https://github.com/apache/incubator-opendal/pull/3492#discussion_r1383308629
##########
bindings/python/python/opendal/exceptions.pyi:
##########
@@ -0,0 +1,86 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+class Error(Exception):
+ """OpenDAL unrelated errors"""
+
+ pass
+
+class UnexpectedError(Exception):
Review Comment:
SGTM
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(binding/python): Add multiple custom exception for each of error code in Rust Core [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3492:
URL: https://github.com/apache/incubator-opendal/pull/3492#issuecomment-1797251763
> I think there should still be a catch-all opendal Exception just like the original Error type, other exceptions should subclass from it, forming an exception hierarchy.
So we will have `opendal.exceptions.Error` and all other exceptions are subclasses of it?
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(binding/python): Add multiple custom exception for each of error code in Rust Core [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3492:
URL: https://github.com/apache/incubator-opendal/pull/3492#discussion_r1383295696
##########
bindings/python/src/errors.rs:
##########
@@ -0,0 +1,86 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use pyo3::create_exception;
+use pyo3::exceptions::PyException;
+
+use crate::*;
+
+create_exception!(opendal, UnexpectedError, PyException, "Unexpected errors");
+create_exception!(
+ opendal,
+ UnsupportedError,
+ PyException,
+ "Unsupported operation"
+);
+create_exception!(
+ opendal,
+ ConfigInvalidError,
+ PyException,
+ "Config is invalid"
+);
+create_exception!(opendal, NotFoundError, PyException, "Not found");
+create_exception!(
+ opendal,
+ PermissionDeniedError,
+ PyException,
+ "Permission denied"
+);
+create_exception!(opendal, IsADirectoryError, PyException, "Is a directory");
+create_exception!(opendal, NotADirectoryError, PyException, "Not a directory");
+create_exception!(opendal, AlreadyExistsError, PyException, "Already exists");
+create_exception!(opendal, IsSameFileError, PyException, "Is same file");
+create_exception!(
+ opendal,
+ ConditionNotMatchError,
+ PyException,
+ "Condition not match"
+);
+create_exception!(
+ opendal,
+ ContentTruncatedError,
+ PyException,
+ "Content truncated"
+);
+create_exception!(
+ opendal,
+ ContentIncompleteError,
+ PyException,
+ "Content incomplete"
+);
+create_exception!(opendal, InvalidInputError, PyException, "Invalid input");
+create_exception!(opendal, Error, PyException, "OpenDAL unrelated errors");
Review Comment:
This error seems not make sense.
##########
bindings/python/python/opendal/exceptions.pyi:
##########
@@ -0,0 +1,86 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+class Error(Exception):
+ """OpenDAL unrelated errors"""
+
+ pass
+
+class UnexpectedError(Exception):
Review Comment:
Is it a good idea to remove the `Error` suffix of class name? So our users will use like:
```python
try:
op.abc()
except opendal.exceptions.Unexpected as err:
raise err
```
##########
bindings/python/src/errors.rs:
##########
Review Comment:
This is a breaking change, please also update `upgrade.md`.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(binding/python): Add multiple custom exception for each of error code in Rust Core [incubator-opendal]
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on PR #3492:
URL: https://github.com/apache/incubator-opendal/pull/3492#issuecomment-1797299489
> So we will have `opendal.exceptions.Error` and all other exceptions are subclasses of it?
Exactly, to catch any opendal exception, user should not need to catch `Exception` because that would also catch non-opendal exceptions.
A good exception hierarchy example is https://github.com/psf/requests/blob/main/src/requests/exceptions.py
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(binding/python): Add multiple custom exception for each of error code in Rust Core [incubator-opendal]
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on code in PR #3492:
URL: https://github.com/apache/incubator-opendal/pull/3492#discussion_r1384390979
##########
bindings/python/upgrade.md:
##########
@@ -27,3 +27,22 @@ Open a file for reading in async way:
async with await op.open(filename, "rb") as r:
content = await r.read()
```
+
+## Breaking change for Errors
+
+We remove the old error classes and provide a couple of Exception based class for the error handling.
+
+1. `opendal.Error` is removed, use `opendal.exceptions.Unexpected` instead.
Review Comment:
This needs an update.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(binding/python): Add multiple custom exception for each of error code in Rust Core [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3492:
URL: https://github.com/apache/incubator-opendal/pull/3492#discussion_r1384422751
##########
bindings/python/tests/test_sync_copy.py:
##########
@@ -16,93 +16,97 @@
# under the License.
import os
-from random import randint
from uuid import uuid4
import pytest
-from opendal.exceptions import IsADirectory, IsSameFile, NotFound
+from opendal.exceptions import Error, IsADirectory, IsSameFile, NotFound
-@pytest.mark.need_capability("read", "write", "copy")
-def test_sync_copy(service_name, operator, async_operator):
+@pytest.mark.need_capability("read", "write", "rename")
+def test_sync_rename_file(service_name, operator, async_operator):
source_path = f"random_file_{str(uuid4())}"
content = os.urandom(1024)
operator.write(source_path, content)
target_path = f"random_file_{str(uuid4())}"
- operator.copy(source_path, target_path)
- read_content = operator.read(target_path)
- assert read_content is not None
- assert read_content == content
- operator.delete(source_path)
+ operator.rename(source_path, target_path)
+ with pytest.raises(Error) as e_info:
+ operator.read(source_path)
+ assert isinstance(e_info.value, NotFound)
+ assert operator.read(target_path) == content
operator.delete(target_path)
+ operator.delete(source_path)
-@pytest.mark.need_capability("read", "write", "copy")
-def test_sync_copy_non_exist(service_name, operator, async_operator):
+@pytest.mark.need_capability("read", "write", "rename")
+def test_sync_rename_non_exists_file(service_name, operator, async_operator):
source_path = f"random_file_{str(uuid4())}"
target_path = f"random_file_{str(uuid4())}"
- with pytest.raises(NotFound) as e_info:
- operator.copy(source_path, target_path)
+ with pytest.raises(Error) as e_info:
+ operator.rename(source_path, target_path)
+ assert isinstance(e_info.value, NotFound)
-@pytest.mark.need_capability("read", "write", "copy")
-def test_sync_copy_source_directory(service_name, operator, async_operator):
+@pytest.mark.need_capability("read", "write", "rename")
+def test_sync_rename_directory(service_name, operator, async_operator):
source_path = f"random_file_{str(uuid4())}/"
operator.create_dir(source_path)
target_path = f"random_file_{str(uuid4())}"
- with pytest.raises(IsADirectory) as e_info:
- operator.copy(source_path, target_path)
+ with pytest.raises(Error) as e_info:
+ operator.rename(source_path, target_path)
+ assert isinstance(e_info.value, IsADirectory)
-@pytest.mark.need_capability("read", "write", "copy")
-def test_sync_copy_target_directory(service_name, operator, async_operator):
+@pytest.mark.need_capability("read", "write", "rename")
+def test_sync_rename_file_to_directory(service_name, operator, async_operator):
source_path = f"random_file_{str(uuid4())}"
content = os.urandom(1024)
operator.write(source_path, content)
target_path = f"random_file_{str(uuid4())}/"
- operator.create_dir(target_path)
- with pytest.raises(IsADirectory) as e_info:
- operator.copy(source_path, target_path)
+ with pytest.raises(Error) as e_info:
Review Comment:
How about adding a unittest instead? Our behavior tests code looks good and don't need to update.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(binding/python): Add multiple custom exception for each of error code in Rust Core [incubator-opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on PR #3492:
URL: https://github.com/apache/incubator-opendal/pull/3492#issuecomment-1797810153
> > So we will have `opendal.exceptions.Error` and all other exceptions are subclasses of it?
>
>
>
> Exactly, to catch any opendal exception, user should not need to catch `Exception` because that would also catch non-opendal exceptions.
>
>
>
> A good exception hierarchy example is https://github.com/psf/requests/blob/main/src/requests/exceptions.py
SGTM
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(binding/python): Add multiple custom exception for each of error code in Rust Core [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3492:
URL: https://github.com/apache/incubator-opendal/pull/3492#discussion_r1383440417
##########
bindings/python/upgrade.md:
##########
@@ -27,3 +27,22 @@ Open a file for reading in async way:
async with await op.open(filename, "rb") as r:
content = await r.read()
```
+
+## Breaking change for Errors
+
+We remove the old error classes and provide a couple of Exception based class for the error handling.
+
+1. `opendal.Error` is removed, use `opendal.exceptions.Unknown` instead.
Review Comment:
I don't like the idea of `Unknown`, please use `Unexpected` instead.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(binding/python): Add multiple custom exception for each of error code in Rust Core [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #3492:
URL: https://github.com/apache/incubator-opendal/pull/3492
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org