Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provide better control over builders object #1426

Open
milyin opened this issue Sep 16, 2024 · 1 comment
Open

provide better control over builders object #1426

milyin opened this issue Sep 16, 2024 · 1 comment
Labels
new feature Something new is needed

Comments

@milyin
Copy link
Contributor

milyin commented Sep 16, 2024

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:

  • return an error immediately. Then my declare_queryable will return `Result<QueryableBuilder, crate::Error>, not same prototype as in zenoh
  • Wrap Result<KeyExpr<‘b>, zenoh::Error> to make it convertable with TryInto<KeyExpr>. This is possible but makes my code very ugly

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

@milyin milyin added the new feature Something new is needed label Sep 16, 2024
@wyfo
Copy link
Contributor

wyfo commented Sep 16, 2024

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Something new is needed
Projects
None yet
Development

No branches or pull requests

2 participants