refactor(core/raw): adjust setter methods of AccessorInfo

[koushiro]

Which issue does this PR close?

No

Rationale for this change

In current usage scenarios, this change will provide a better experience in method chaining,
although it will result in a breaking change.

What changes are included in this PR?

adjust setter methods of AccessorInfo

• remove:
  • fn full_capability_mut(&mut self) -> &mut Capability
• add:
  • fn with_full_capability(mut self, capability: Capability) -> Self
• change:
  • fn set_scheme(&mut self, scheme: Scheme) -> &mut Self => fn with_scheme(mut self, scheme: Scheme) -> Self
  • fn set_root(&mut self, root: &str) -> &mut Self => fn with_root(mut self, root: &str) -> Self
  • fn set_name(&mut self, name: &str) -> &mut Self => fn with_name(mut self, name: &str) -> Self
  • fn set_native_capability(&mut self, capability: Capability) -> &mut Self => fn with_native_capability(mut self, capability: Capability) -> Self

Are there any user-facing changes?

setter methods of AccessorInfo struct

[Xuanwo]

• fn set_scheme(&mut self, scheme: Scheme) -> &mut Self => fn set_scheme(mut self, scheme: Scheme) -> Self
• fn set_root(&mut self, root: &str) -> &mut Self => fn set_root(mut self, root: &str) -> Self
• fn set_name(&mut self, name: &str) -> &mut Self => fn set_name(mut self, name: &str) -> Self
• fn set_native_capability(&mut self, capability: Capability) -> &mut Self => fn set_native_capability(mut self, capability: Capability) -> Self

Hi, this is our API naming style that:

• fn abc(&self) -> Abc
• fn set_abc(&mut self, v: Abc) -> &mut Self
• fn with_abc(mut self, v: Abc) -> Self.

[koushiro]

Hi, this is our API naming style that:

• fn abc(&self) -> Abc
• fn set_abc(&mut self, v: Abc) -> &mut Self
• fn with_abc(mut self, v: Abc) -> Self.

@Xuanwo So do you think this change is ok, except for the API naming style?

[Xuanwo]

So do you think this change is ok, except for the API naming style?

I believe we only need to add set_full_capability and mark full_capability_mut as deprecated. Other changes seem not needed.

[Xuanwo]

Hi, sorry for not expressing my idea correctly. Breaking changes like 5fdd515 (#5244) just to alter API naming don't seem good to me.

[Xuanwo]

In general, we don't want to introduce breaking changes to the API unless there are compelling reasons to do so. For example, such changes might significantly improve our readability, or they might be necessary for implementing certain features.

[koushiro]

In general, we don't want to introduce breaking changes to the API unless there are compelling reasons to do so. For example, such changes might significantly improve our readability, or they might be necessary for implementing certain features.

but for now, AccessorInfo has setter APIs with &mut self as parameter and return &mut Self as return value, IMV, they don't work well with method chaining in current use cases, like below:

• Before this PR:

    fn info(&self) -> Arc<AccessorInfo> {
        let mut am = AccessorInfo::default();
        am.set_scheme(Scheme::AliyunDrive)
            .set_root(&self.core.root)
            .set_native_capability(Capability {
                stat: true,
                create_dir: true,
                read: true,
                write: true,
                write_can_multi: true,
                // The min multipart size of AliyunDrive is 100 KiB.
                write_multi_min_size: Some(100 * 1024),
                // The max multipart size of AliyunDrive is 5 GiB.
                write_multi_max_size: if cfg!(target_pointer_width = "64") {
                    Some(5 * 1024 * 1024 * 1024)
                } else {
                    Some(usize::MAX)
                },
                delete: true,
                copy: true,
                rename: true,
                list: true,
                list_with_limit: true,

                ..Default::default()
            });
        am.into()
    }

• After this PR

    fn info(&self) -> Arc<AccessorInfo> {
        AccessorInfo::default()
            .with_scheme(Scheme::AliyunDrive)
            .with_root(&self.core.root)
            .with_native_capability(Capability {
                stat: true,
                create_dir: true,
                read: true,
                write: true,
                write_can_multi: true,
                // The min multipart size of AliyunDrive is 100 KiB.
                write_multi_min_size: Some(100 * 1024),
                // The max multipart size of AliyunDrive is 5 GiB.
                write_multi_max_size: if cfg!(target_pointer_width = "64") {
                    Some(5 * 1024 * 1024 * 1024)
                } else {
                    Some(usize::MAX)
                },
                delete: true,
                copy: true,
                rename: true,
                list: true,
                list_with_limit: true,

                ..Default::default()
            })
            .into()
    }

[Xuanwo]

IMV, they don't work well with method chaining in current use cases, like below:

It's fine since this API has never been exposed to users. (Please note it's part of the raw API and not our public API surface.)

It's not that bad, right? These APIs are only written once per service and are rarely modified.

Perhaps we can discuss this when we are ready for the OpenDAL core v1.0 release. At this time, introducing breaking changes isn't worth it.

---

It might be better to initiate a discussion about any API changes before implementing them. I'm glad that someone like you, @koushiro, is willing to take the time to work on this, but we always need to balance the new API changes with the existing API.

[koushiro]

BTW, I found that the API of trait Access is a little different from trait LayeredAccess.
Is this intentional or caused by misaligned changes?

pub trait Access: Send + Sync + Debug + Unpin + 'static {
    fn info(&self) -> Arc<AccessorInfo>;
}

pub trait LayeredAccess: Send + Sync + Debug + Unpin + 'static {
    fn metadata(&self) -> Arc<AccessorInfo> {
        self.inner().info()
    }
}

And the similar inconsistencies exist in the APIs of kv::Adapter and typed_kv::Adapter.

• kv::Adapter

pub trait Adapter: Send + Sync + Debug + Unpin + 'static {
    /// Return the metadata of this key value accessor.
    fn metadata(&self) -> Metadata;
}

/// Metadata for this key value accessor.
pub struct Metadata {
    scheme: Scheme,
    name: String,
    capabilities: Capability,
}

• typed_kv::Adapter

pub trait Adapter: Send + Sync + Debug + Unpin + 'static {
    /// Get the scheme and name of current adapter.
    fn info(&self) -> Info;
}

/// Info for this key value accessor.
pub struct Info {
    scheme: Scheme,
    name: String,
    capabilities: Capability,
}

[Xuanwo]

BTW, I found that the API of trait Access is a little different from trait LayeredAccess. Is this intentional or caused by misaligned changes?

Yes, I believe it's misaligned. We used to use metadata and changed it to info.

And the similar inconsistencies exist in the APIs of kv::Adapter and typed_kv::Adapter.

Keen eyes! They are inconsistent for the same reason. I would appreciate it if you could help fix them.

[Xuanwo]

Hi, @koushiro. I'm going to close this PR as we don't need it for now. Feel free to propose a discussion if you think there has been a misunderstanding.