Proposal for fix for loss of relation between message and ack_id.

[olofwalker]

This PR attempts a fix for #14

This changes the returned values from get_messages from a tuple of vectors with ack_ids and message to a HashMap where ack_id is the key of a Result that contains either the successfully deserialized message or the error that occurred.

[hseeberger]

@kayleg may I kindly ask whether you already had time to consider this PR?

[kayleg]

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.

[hseeberger]

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?

[hseeberger]

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> { ... }

[kayleg]

I like your last point, we're not using any features provided by the HashMap so let's just return the vec of tuples.

[hseeberger]

Hey @kayleg, it would be great if you could review this again.

[kayleg]

LGTM - Thanks folks. I'll cut a new release including this.