eclipse-zenoh / roadmap

Other
26 stars 5 forks source link

Update Query Payload.md #64

Closed p-avital closed 1 year ago

p-avital commented 1 year ago

Proposing to remove with_value_opt, whose only apparent value is the ability to remove a possibly existing value, which a take-like API is better suited for

Mallets commented 1 year ago

Using only the with_value makes a builder patter more clean to me and easier to grasp what's going on under the hood. Having the user adding an if doesn't shock me.

I'm not sure about the benefit of take_value API on a query. I would see rather on the Query received by a queryable.

Mallets commented 1 year ago

A second comment regarding the name: in my opinion would be easier to understand if we associate the name body to a query. This makes it very similar to HTTP queries people are already familiar with.

What about then calling it with_body?

p-avital commented 1 year ago

A second comment regarding the name: in my opinion would be easier to understand if we associate the name body to a query. This makes it very similar to HTTP queries people are already familiar with.

What about then calling it with_body?

Samples already have payloads, so I'd be in favor of payload rather than body :)

OlivierHecart commented 1 year ago

I called it with_value as it takes as param an actual Zenoh Value.

The payload accessor in Sample is a shortcut for Sample.value().payload() and actually returns a buffer not a Value. About HTTP, setting a body in a GET request is possible but "discouraged".

Mallets commented 1 year ago

Ok, I'm fine staying with_value then :)

Mallets commented 1 year ago

For the z_get example, I would rather write it like:

let replies = if let Some(v) = v {
    session.get("key").with_value(v).res().unwrap()
} else {
    session.get("key").res().unwrap()
};