sociomantic-tsunami / dlsproto

Distributed Log Store protocol definition, client, fake node, and tests
Boost Software License 1.0
3 stars 18 forks source link

Replace buffer used for message formatting in notifiers #35

Closed daniel-zullo closed 6 years ago

daniel-zullo commented 6 years ago

The IClient already provides a buffer for message formatting and it should be used in the examples instead of defining there a new buffer.

gavin-norman-sociomantic commented 6 years ago

There are a couple of reasons for not using the buffer owned by IClient:

gavin-norman-sociomantic commented 6 years ago

Question: do you find it actually confusing that the usage examples don't use the IClient buffer?

daniel-zullo commented 6 years ago

do you find it actually confusing that the usage examples don't use the IClient buffer?

I don't find it confusing but redundant. Why I should define a new buffer for message formatting in the notifier when the IClient already provides a buffer for such a thing? Moreover I cannot see any message in the documentation saying Do not use this buffer and it is not even deprecated.

Using a plain mstring is slightly clearer, in a usage example. (It doesn't hint at secret features of the client.)

TBH I don't see it as a secret feature of the client. It is visible there and well documented. https://github.com/sociomantic-tsunami/swarm/blob/v4.x.x/src/swarm/client/model/IClient.d#L164-L170

IClient and everything in it will cease to exist, at some point. (When the legacy stuff is finally removed.)

If that is the plan, then IClient should have been either deprecated or well documented at least to prevent different projects to continue using functionality from it. At the current state I find it difficult to figure out what features will remain and cease as a user of the project.

gavin-norman-sociomantic commented 6 years ago

You're misunderstanding what I'm saying.

IClient is currently not deprecated and you are free to use all of its features (including this buffer). (Using it is, indeed, essential, as all of the legacy client features stem from IClient.)

The point is that IClient is the legacy client base class and will be deprecated once we're at the point of moving entirely to neo. This is why I don't especially want to adapt neo usage examples to rely on features of the legacy client base class.

See what I mean?

gavin-norman-sociomantic commented 6 years ago

We may well add a buffer like this to the neo client, as well. If that happens, we could adapt the usage examples to use it.

daniel-zullo commented 6 years ago

Yes, now I see what you mean. Then I'm closing this PR. And thanks for the clarification.