datrs / hypercore

Secure, distributed, append-only log
https://docs.rs/hypercore
Apache License 2.0
327 stars 37 forks source link

Remove Feed Builder in favor of Feed simple create function #72

Open FreddieRidell opened 5 years ago

FreddieRidell commented 5 years ago

https://github.com/datrs/hypercore/blob/482f491c401b2a0c6079022d742f761761ee8079/src/feed_builder.rs#L14-L18

The link referenced shows a crate that offers a macro to automatically derive builders for structs. It seems like it should be relatively easy to use derive_builder with Feed. Is the intention to use this macro, or write a custom builder?

yoshuawuyts commented 5 years ago

@FreddieRidell I was thinking a custom builder, as we're able to have more control over it.

FreddieRidell commented 5 years ago

Looking through the FeedBuilder, it seems that byte_length should be being set to the total number of bytes already recorded in the Storage? this can be done, but would require updating to the newest versions of update-random-access-*.

Have I understood this correctly?

yoshuawuyts commented 5 years ago

it seems that byte_length should be being set to the total number of bytes already recorded in the Storage

Though my memory is a bit foggy on the matter, I believe that's accurate.

ZhouHansen commented 4 years ago

Is there an actual builder pattern? I tried to make it an Non-consuming builder, then we can write:

if partial_keypair.secret.is_some() {
    builder.secret_key(partial_keypair.secret.unwrap())
}

Ok(builder.build()?)

But the lifetime doesn't allow it. But it is ok:

Under the rubric of making easy things easy and hard things possible, all builder methods for a consuming builder should take and returned an owned self.

Builder-pattern is useful when build a object takes a lot of work. Maybe we can build feed directly here? I opened a pr for it.

yoshuawuyts commented 4 years ago

@ZhouHansen I'm not sure I follow. The feed_builder is an "actual" builder. The way to write the code above is:

if partial_keypair.secret.is_some() {
    builder = builder.secret_key(partial_keypair.secret.unwrap())
}

Ok(builder.build()?)

Maybe I'm missing something. But whatever the problem is, it seems unlikely that removing the builder in its entirety would be the solution.

ZhouHansen commented 4 years ago

@yoshuawuyts I see the // TODO: in the comment, this misleads me that the current feed_builder is not an "actual" builder yet and there're more works to do here.

At the same time I feel it's unnecessary to use builder-pattern here, the building process of feed is not complex.

bltavares commented 4 years ago

As part of the discussion, it seems like the Builder is an actual builder.

In regards to the Feed being simple: check my comment on the PR https://github.com/datrs/hypercore/pull/80#issuecomment-629823538