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

move Publisher.write method into trait #736

Merged

Conversation

DenisBiryukov91
Copy link
Contributor

move Publisher.write method into HasWriteWithSampleKind trait (closes #731 )

@Mallets Mallets requested a review from p-avital February 13, 2024 16:25
Copy link
Contributor

@p-avital p-avital left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we also plan to implement this trait for other types, I'm against this PR as a concept: it's a breaking change, makes finding this capability of the Publisher more complex, makes the doc worse by splitting it, and doesn't bring any code reuse benefits.

If we insist that we want to do this, then this PR's code is acceptable, but like I said, I don't think we want to do this.

@Mallets
Copy link
Member

Mallets commented Feb 16, 2024

The very reason for the write method to exist is a niche case of Stream-like interface where a subscriber can be piped into a subscriber. The very existence of write already generated some confusion on what's the core API of a publisher and what is available in other bindings. I believe we should separate it in a different trait, we could then argue about the naming...

@p-avital
Copy link
Contributor

To me, separating it doesn't make the API any simpler, so I find the cost of an API break to high.

Splitting a method off into a trait makes sense when there may be multiple implementors, or when adding methods to a foreign type. I don't think it's helpful to "hide" methods, especially given that this one's semantic is far from complex.

My reasoning is that this has no benefits (adding indirections in the doc is not one IMO), so there's no reason to break this API.

A much greater improvement IMO would be to change the doc to something like:

[`put`](Publisher::put) or [`delete`](Publisher::delete) based on the [`SampleKind`].

An example of this being useful is forwarding received [`Samole`]s without checking whether it's a put or a delete.

This would make the use-case and alternatives much easier to understand than sweeping the API under the rug.

@Mallets
Copy link
Member

Mallets commented Feb 16, 2024

Again, my point is that method does NOT exist and it makes the API asymmetrical. This an API whose sole reason to exist is for some Rust ergonomics. I tend to disagree with Splitting a method off into a trait makes sense when there may be multiple implementors, it can also be use to explicitly group a set of methods under a given trait.

What we want to convey here is: this is an API specific to Rust, not a generic Zenoh Publisher. Another way to see it, is that this API should implement something like StreamExt. I'm not sure we have the bandwidth to do a full StreamExt implementation for the publisher and a subscriber right now...

@milyin
Copy link
Contributor

milyin commented Mar 12, 2024

I'd support @p-avital. I'm also kind of against to the idea of using traits as a way to separate API to "first-class" and "second-class" elements.
At this moment we made a list of "core" api elements (https://github.com/eclipse-zenoh/zenoh/blob/tagging/api_core.txt), so the case of write is not unique. We have multiple functions which are part of public rust API, but not part of core API. So I see no problem to leave the writein publisher impl.

@Mallets
Copy link
Member

Mallets commented Mar 13, 2024

Write accepts a SampleKind and a Value, which is a duplication of already existing put and delete operations on the publisher. Is there any real usage for that or is it just a redundant API with not much added value? I'd be more in favour now to remove it.

@milyin milyin mentioned this pull request Mar 13, 2024
@milyin
Copy link
Contributor

milyin commented Mar 13, 2024

Agree with @Mallets. The delete operation is not normally supposed to contain payload. So it's better to not expose the ability to make it, even if protocol can do it now

@Mallets Mallets merged commit ba50eec into eclipse-zenoh:main Mar 13, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write in Rust Publisher should be moved to a trait
4 participants