Closed olofwalker closed 2 years ago
@kayleg may I kindly ask whether you already had time to consider this PR?
Apologies for the delay - thanks for adding this.
My one question is: Do we like having to check each message for an error?
I personally like just being able to operate over the results without the needed boilerplate of
if let Ok(m) = someResult
but without having each entry as a result we can't correctly handle invalid input.
@olofwalker and @hseeberger Let me know your thoughts. I'll merge it in if we're happy with the resulting interface.
Thanks for getting back so quickly. Unfortunately I do not fully understand your question. So let me explain what we need: after pulling a message from Pub/Sub, we apply further processing which might fail or not. Therefore we need to know exactly which ack_id is associated with a particular message. That is the essence of this PR. Whether we use a HashMap from ack_id to message or vice versa or a vector of tuples does not matter that much. Actually one might argue that we should switch to a vector of tuples, because we do not use any of the HashMap features.
Does that answer your question?
After some more thinking I guess I know understand your question. As it is unclear whether users want to acknowledge Pub/Sub messages which could not get deserialized or not, I think we must return all results with their associated ack_ids. For our use case it would be fine to filter out the errors and only return all successfully deserialized messages with their ack_ids, but that is just our current use case.
Nevertheless I think that according to the principle of least power we should not return a HashMap but a Vec<(String, Result<T, error::Error>)>
instead. Or maybe better flip the tuple:
pub async fn get_messages<T: FromPubSubMessage>(&self) ->
Result<Vec<(Result<T, error::Error>, String)>, error::Error> { ... }
I like your last point, we're not using any features provided by the HashMap
so let's just return the vec of tuples.
Hey @kayleg, it would be great if you could review this again.
This PR attempts a fix for #14
This changes the returned values from
get_messages
from a tuple of vectors withack_ids
and message to aHashMap
whereack_id
is the key of aResult
that contains either the successfully deserialized message or the error that occurred.