You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "koushiro (via GitHub)" <gi...@apache.org> on 2025/11/08 13:40:45 UTC
[PR] refactor(types): use string-based scheme over enum-based approach [opendal]
koushiro opened a new pull request, #6765:
URL: https://github.com/apache/opendal/pull/6765
# Which issue does this PR close?
Part of #6755
# Rationale for this change
After merging #6764, the `Scheme` enum is no longer very useful, and is only used for the `Operator::via_iter` and `Operator::via_map` APIs. The `via_map` API has been replaced by `via_iter` and is now deprecated, while the `via_iter` API is used more in test scenarios.
# What changes are included in this PR?
- remove `Scheme` enum
# Are there any user-facing changes?
- remove `Scheme` enum
- `Operator::via_iter(scheme: Scheme, iter: impl IntoIterator<Item = (String, String)>)` => `Operator::via_iter(scheme: &str, iter: impl IntoIterator<Item = (String, String)>)`
- `Operator::via_map(scheme: Scheme, map: HashMap<String, String>)` => `Operator::via_map(scheme: &str, map: HashMap<String, String>)`
- `OperatorInfo::scheme(&self) -> Scheme` => `Operator::scheme(&self) -> &'static str`
--
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(types): use string-based scheme over enum-based approach [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on code in PR #6765:
URL: https://github.com/apache/opendal/pull/6765#discussion_r2511151001
##########
core/src/services/aliyun_drive/mod.rs:
##########
@@ -16,7 +16,7 @@
// under the License.
/// Default scheme for aliyun_drive service.
-pub const ALIYUN_DRIVE_SCHEME: &str = "aliyun-drive";
+pub const ALIYUN_DRIVE_SCHEME: &str = "aliyun_drive";
Review Comment:
I unified the connectors in the scheme string to `-`.
There are some errors in the behavior tests/core in CI, because now we have started using a string-based scheme. However, the current minimoka service uses `mini-moka` as the scheme, while the `test_behavior_core.yml` workflow uses the name from `".github/services/mini_moka/mini_moka"` to set the `OPENDAL_TEST` env variable.
@Xuanwo Do you think I should rename these path names in `.github/services`, or modify the script in the workflow to replace `_` with `-` in the scheme string passed via the `OPENDAL_TEST` env variable?
--
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(types): use string-based scheme over enum-based approach [opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #6765:
URL: https://github.com/apache/opendal/pull/6765#discussion_r2507606411
##########
core/src/services/aliyun_drive/mod.rs:
##########
@@ -16,7 +16,7 @@
// under the License.
/// Default scheme for aliyun_drive service.
-pub const ALIYUN_DRIVE_SCHEME: &str = "aliyun-drive";
+pub const ALIYUN_DRIVE_SCHEME: &str = "aliyun_drive";
Review Comment:
Why does this flavor change? If it's unnecessary, we may not do 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(types): use string-based scheme over enum-based approach [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on code in PR #6765:
URL: https://github.com/apache/opendal/pull/6765#discussion_r2507613167
##########
core/src/services/aliyun_drive/mod.rs:
##########
@@ -16,7 +16,7 @@
// under the License.
/// Default scheme for aliyun_drive service.
-pub const ALIYUN_DRIVE_SCHEME: &str = "aliyun-drive";
+pub const ALIYUN_DRIVE_SCHEME: &str = "aliyun_drive";
Review Comment:
If we plan to remove the `Scheme` enum later and use a unique string as the scheme, it'd be best to standardize on a certain connector.
The reason some existing CIs haven't encountered errors is that they convert the connectors as follows,
```rust
/// Init a service with given scheme.
///
/// - Load scheme from `OPENDAL_TEST`
/// - Construct a new Operator with given root.
/// - Else, returns a `None` to represent no valid config for operator.
pub fn init_test_service() -> Result<Option<Operator>> {
let _ = dotenvy::dotenv();
let scheme = if let Ok(v) = env::var("OPENDAL_TEST") {
v
} else {
return Ok(None);
};
let scheme = Scheme::from_str(&scheme).unwrap();
let scheme_key = String::from(scheme).replace('-', "_");
let prefix = format!("opendal_{scheme_key}_");
...
}
```
and the `FromStr` implementation of `Scheme` enum also matches multiple formats in the following ways:
```rust
impl FromStr for Scheme {
type Err = Error;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let s = s.to_lowercase();
match s.as_str() {
"aliyun-drive" | "aliyun_drive" => Ok(Scheme::AliyunDrive),
...
}
}
```
--
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(types): use string-based scheme over enum-based approach [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #6765:
URL: https://github.com/apache/opendal/pull/6765#discussion_r2513445435
##########
core/src/services/aliyun_drive/mod.rs:
##########
@@ -16,7 +16,7 @@
// under the License.
/// Default scheme for aliyun_drive service.
-pub const ALIYUN_DRIVE_SCHEME: &str = "aliyun-drive";
+pub const ALIYUN_DRIVE_SCHEME: &str = "aliyun_drive";
Review Comment:
> modify the script in the workflow to replace `_` with `-` in the scheme string passed via the `OPENDAL_TEST` env variable?
This one seems better and easier?
--
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(types)!: use string-based scheme over enum-based approach [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #6765:
URL: https://github.com/apache/opendal/pull/6765#issuecomment-3521299840
> @koushiro seems like some changes will have to be done to the python bindings as well. I'll take this up as part of #6748
Thank you!
--
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(types): use string-based scheme over enum-based approach [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #6765:
URL: https://github.com/apache/opendal/pull/6765#discussion_r2509527919
##########
core/src/services/aliyun_drive/mod.rs:
##########
@@ -16,7 +16,7 @@
// under the License.
/// Default scheme for aliyun_drive service.
-pub const ALIYUN_DRIVE_SCHEME: &str = "aliyun-drive";
+pub const ALIYUN_DRIVE_SCHEME: &str = "aliyun_drive";
Review Comment:
I think it's better to keep the `aliyun-drive` as scheme since `_` is not allowed in spec: https://developer.mozilla.org/en-US/docs/Web/URI/Reference/Schemes
--
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(types): use string-based scheme over enum-based approach [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on PR #6765:
URL: https://github.com/apache/opendal/pull/6765#issuecomment-3507524304
To avoid too many breaking changes to bindings, I don't want to remove the `Scheme` enum in this PR.
--
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(types)!: use string-based scheme over enum-based approach [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #6765:
URL: https://github.com/apache/opendal/pull/6765#issuecomment-3520752644
Release failed anyways...
Let's merge now.
--
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(types)!: use string-based scheme over enum-based approach [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #6765:
URL: https://github.com/apache/opendal/pull/6765
--
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(types)!: use string-based scheme over enum-based approach [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on code in PR #6765:
URL: https://github.com/apache/opendal/pull/6765#discussion_r2517451525
##########
core/src/types/scheme.rs:
##########
@@ -34,63 +33,66 @@ use crate::Error;
pub enum Scheme {
Review Comment:
Actually, some bindings still rely on the `Scheme` enum (like java bindings).
The current PR only removes the usage of the `Scheme` enum from the core.
--
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(types)!: use string-based scheme over enum-based approach [opendal]
Posted by "chitralverma (via GitHub)" <gi...@apache.org>.
chitralverma commented on PR #6765:
URL: https://github.com/apache/opendal/pull/6765#issuecomment-3521290401
@koushiro seems like some changes will have to be done to the python bindings as well.
I'll take this up as part of https://github.com/apache/opendal/issues/6748
--
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(types): use string-based scheme over enum-based approach [opendal]
Posted by "koushiro (via GitHub)" <gi...@apache.org>.
koushiro commented on code in PR #6765:
URL: https://github.com/apache/opendal/pull/6765#discussion_r2513520624
##########
core/src/services/aliyun_drive/mod.rs:
##########
@@ -16,7 +16,7 @@
// under the License.
/// Default scheme for aliyun_drive service.
-pub const ALIYUN_DRIVE_SCHEME: &str = "aliyun-drive";
+pub const ALIYUN_DRIVE_SCHEME: &str = "aliyun_drive";
Review Comment:
It might be better to directly modify the `init_test_service` function?
--
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(types)!: use string-based scheme over enum-based approach [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #6765:
URL: https://github.com/apache/opendal/pull/6765#issuecomment-3516032391
Let's merge this PR after `0.55` been released.
--
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(types)!: use string-based scheme over enum-based approach [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #6765:
URL: https://github.com/apache/opendal/pull/6765#discussion_r2517579715
##########
core/src/types/scheme.rs:
##########
@@ -34,63 +33,66 @@ use crate::Error;
pub enum Scheme {
Review Comment:
> Actually, some bindings still rely on the `Scheme` enum (like java bindings). The current PR only removes the usage of the `Scheme` enum from the core.
Got it, we can finally remove them all in the future.
--
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