refactor(core): Deprecate OpList::version and add versions instead #5481
Conversation
| /// | ||
| /// Default to `false` | ||
| pub fn versioned(self, v: bool) -> Self { | ||
| self.map(|args| args.with_version(v)) |
There was a problem hiding this comment.
Please also deprecate all use of OpList::version and change them to OpList::versioned.
Xuanwo
changed the title
There was a problem hiding this comment.
Hi, @geetanshjuneja, I apologize for not listing all the list_with_version patterns.
The OpenDAL API is a consistent system, so all fields like list_with_version should be deprecated in favor of list_with_versioned.
core/src/layers/capability_check.rs
Outdated
| @@ -191,7 +191,7 @@ impl<A: Access> LayeredAccess for CapabilityAccessor<A> { | |||
| args: OpList, | |||
| ) -> crate::Result<(RpList, Self::BlockingLister)> { | |||
| let capability = self.info.full_capability(); | |||
| if !capability.list_with_version && args.version() { | |||
| if !capability.list_with_version && args.versioned() { | |||
There was a problem hiding this comment.
list_with_version should also be depreated by list_with_versioned.
There was a problem hiding this comment.
No problem I'll add that all also.
core/src/raw/ops.rs
Outdated
| pub fn version(&self) -> bool { | ||
| self.version | ||
| } | ||
|
|
||
| /// Get the version of this list operation | ||
| pub fn versioned(&self) -> bool { |
There was a problem hiding this comment.
with_version should be replaced by with_versioned.
| @@ -486,9 +486,21 @@ impl<F: Future<Output = Result<Vec<Entry>>>> FutureList<F> { | |||
| /// by the underlying service | |||
| /// | |||
| /// Default to `false` | |||
| #[deprecated = "use versioned instead"] | |||
There was a problem hiding this comment.
Please also add a version field: 0.51.1
core/src/raw/ops.rs
Outdated
|
|
||
| /// Get the version of this list operation | ||
| pub fn versioned(&self) -> bool { | ||
| self.version |
There was a problem hiding this comment.
Should be changed by self.versioned
core/src/types/capability.rs
Outdated
| @@ -193,7 +193,7 @@ pub struct Capability { | |||
| /// Indicates whether last modified timestamp is available in list response | |||
| pub list_has_last_modified: bool, | |||
| /// Indicates whether version information is available in list response | |||
| pub list_has_version: bool, | |||
| pub list_has_versioned: bool, | |||
There was a problem hiding this comment.
This doesn't need to change. the version is the version in metadata, not for the versioned filter in list operation.
core/src/types/capability.rs
Outdated
| @@ -175,7 +175,7 @@ pub struct Capability { | |||
| /// Indicates if recursive listing is supported. | |||
| pub list_with_recursive: bool, | |||
| /// Indicates if versioned listing is supported. | |||
| pub list_with_version: bool, | |||
| pub list_with_versioned: bool, | |||
There was a problem hiding this comment.
Capability is part of opendal's public API so we can't simplely change it. Please add a new field and deprecate the old one.
| @@ -529,6 +541,6 @@ impl<F: Future<Output = Result<Lister>>> FutureLister<F> { | |||
| /// | |||
| /// Default to `false` | |||
| pub fn version(self, v: bool) -> Self { | |||
| self.map(|args| args.with_version(v)) | |||
| self.map(|args| args.with_versioned(v)) | |||
There was a problem hiding this comment.
Please add a versioned for it too. Also, this API should be deprecated.
There was a problem hiding this comment.
Thank you, @geetanshjuneja, for this change; it mostly looks good to me now! I consulted with some native English speakers, and they all felt that list_with(path).versioned(true) was a bit confusing. Therefore, I will push another change to use versions(true) instead.
Xuanwo
changed the title
Signed-off-by: Xuanwo <[email protected]>
Which issue does this PR close?
Closes #5476.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?