Sample accessors #822
Sample accessors #822
Conversation
|
@DenisBiryukov91 If this pull request contains a bugfix or a new feature, then please consider using |
| pub fn with_timestamp(mut self, timestamp: Timestamp) -> Self { | ||
| self.timestamp = Some(timestamp); | ||
| self | ||
| } | ||
|
|
||
| /// Gets the quality of service settings this Sample was sent with. | ||
| #[inline] | ||
| pub fn qos(&self) -> &QoS { |
There was a problem hiding this comment.
In the put and delete builders we have explicit options to set the Priority and CongestionControl. I understand that this PR only addresses the accessor pattern and it does not modify the types exposed. I wonder if we should have priority() and congestion_control() instead of a generic qos()?
There was a problem hiding this comment.
Depends whether we want to provide an absolutely flat structure, or if we are ok, to group some related values under a dedicated accessor, to avoid polluting ide autocompletion list. We should also probably ask ourselves whether congestion control has something to do with the sample from design point of view ? It seems to me put(...).with_congestion_control(...) makes a bit more sense than sample.congestion_control().
There was a problem hiding this comment.
Why it makes a bit more sense on the put(...) than on the sample? Given the sample contains all the information associated to the data + publication?
There was a problem hiding this comment.
put(ke, payload).with_priority(...).with_congestion_control(...) corresponds to the phrase : "publish this payload on this ke with specified priority and congestion control [values]".
sample.priority() - corresponds to the phrase: "[Give me] priority value this sample was published with. Compare this for example with sample.encoding() - which corresponds to "[Give me] encoding (or format) of this sample". As you can see in the latter case (and in case of put) we do not need to add any extra words (except prepositions/pronouns) beyond the ones present in the function call. This is different for sample.priority(). I believe as long as we need to add extra words to describe what a line of code does, we ought to add extra layers of indirection.
There was a problem hiding this comment.
I'd like to quickly remark that the API and accessors should be the same on all bindings and languages, I wouldn't add a level of indirection in C for example.
There was a problem hiding this comment.
maybe we can have something like z_sample_qos_priority(sample) ?
|
@DenisBiryukov91 Can you link this to its relevant issue? If there's no yet, then you can create one using the release template if this is meant to be included in the next release. Make sure also to add the proper timelines |
|
Integrating this. Further API changes, like flattening qos accessors, is easier to apply over this update |
make Sample fields pub(crate),
provide accessors for external users,
mark existing sample-mutating methods as hidden and unstable.
Closes #817.