eclipse-zenoh / zenoh

zenoh unifies data in motion, data in-use, data at rest and computations. It carefully blends traditional pub/sub with geo-distributed storages, queries and computations, while retaining a level of time and space efficiency that is well beyond any of the mainstream stacks.
https://zenoh.io
Other
1.44k stars 151 forks source link

provide better control over builders object #1426

Open milyin opened 2 weeks ago

milyin commented 2 weeks ago

Describe the feature

I have one concern about our prototypes for “declare_queryable” and similar functions

I’m trying to implement my wrapper with the same API as in zenoh and I found that I can’t do it well

The prototype of declare_queryable is

    fn declare_queryable<'b, TryIntoKeyExpr>(
        &'a self,
        key_expr: TryIntoKeyExpr,
    ) -> QueryableBuilder<'a, 'b, DefaultHandler>
     where
        TryIntoKeyExpr: TryInto<KeyExpr<'b>>,
        <TryIntoKeyExpr as TryInto<KeyExpr<'b>>>::Error: Into<zenoh_result::Error>,

I want to make a wrapper which does some conversion on key expression passed - something like “self.base_keyexpr.concat(&keyexpr)?”). After this I have 2 choices, both bad:

The real problem is incompleteness of our Builders API. The builder is allowed to return an error on final stage, but there is no way to set this error explicitly, though sometimes it’s necessary to.

The proposal is to adding explicit methods for constructing builders and assigning errors and session to them. I.e current builder structure is

pub struct QueryableBuilder<'a, 'b, Handler> {
    pub(crate) session: SessionRef<'a>,
    pub(crate) key_expr: ZResult<KeyExpr<'b>>,
    pub(crate) complete: bool,
    pub(crate) origin: Locality,
    pub(crate) handler: Handler,
}

The new proposed variant is:

pub struct QueryableBuilder<'a, 'b, Handler> {
    pub(crate) session: Option<SessionRef<'a>>,
    pub(crate) key_expr: Option<KeyExpr<'b>>>,
    pub(crate) complete: bool,
    pub(crate) origin: Locality,
    pub(crate) handler: Handler,
    pub(crate) error: Option<zenoh::Error>,
}

And also construction and setting error will be implemented in the BulderConstructionTrait, so without importing it the normal builder usage is not affected. This trait will be “unstable” initially of course

pub trait BuilderConstructionTrait {
    fn new() -> Self;
    fn session(self, session: Session) -> Self;
    fn keyexpr(self, keyexpr: KeyExpr) -> Self;
    fn error(self, error: zenoh::Error) -> Self;
}

This will allow to build the builder object explicitly with full control on it’s behaviour which is sometimes necessary for library developers

wyfo commented 2 weeks ago

Wrap Result<KeyExpr<‘b>, zenoh::Error> to make it convertable with TryInto. This is possible but makes my code very ugly

I prefer making your code ugly instead of making the zenoh API more complex (and maybe less performant) for one specific use case that doesn't concern most of our users. (And I'm not even sure your code would be so ugly)