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

Make the parser Response be public #40

Closed
wants to merge 1 commit into from
Closed

Conversation

drinkbeer
Copy link
Contributor

@drinkbeer drinkbeer commented Sep 19, 2024

TL;DR - What are you trying to accomplish?

Expose the Response which is part of the returned hashmap of set_multi API.

Details - How are you making this change? What are the effects of this change?

@drinkbeer drinkbeer marked this pull request as ready for review September 19, 2024 04:08
@drinkbeer drinkbeer requested a review from a team as a code owner September 19, 2024 04:08
Copy link
Contributor

@nickamorim nickamorim left a comment

Choose a reason for hiding this comment

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

Should we instead change set_multi to return a Result<FxHashMap<&'a K, Result<Status, Error>>, Error>?

@mrattle
Copy link
Contributor

mrattle commented Sep 20, 2024

Should we instead change set_multi to return a Result<FxHashMap<&'a K, Result<Status, Error>>, Error>?

I think we get the same information from just simplifying even further to return Result<FxHashMap<&'a K, Result<(), Error>>, Error>. I think the only green-path response that we can get back on a set operation is STORED, and everything else would be parsed into an Err(_) variant. This is matches with how the single set operation is implemented, and would also hold true for the add_multi operation where NOT_STORED can be returned but is parsed as an error.

@mrattle
Copy link
Contributor

mrattle commented Sep 20, 2024

Now that #41 is merged, this PR can be closed.

@mrattle mrattle closed this Sep 20, 2024
@mrattle mrattle deleted the jchen/public_response branch September 26, 2024 14:24
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.

3 participants