react: return domain-specific errors (DO NOT MERGE, reference for #434 #464 merge ONLY) #463
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**Problem statement**: Consider a test with two `set_value` calls and which expects that a callback has, ultimately, been called with the two values, one for each `set_value`. The tests currently do not check that one value was added during each `set_value` call. For all we know, maybe an implementation: * magically manages to predict the future and calls the callback twice on the first `set_value` call, with the correct value. * calls the callback zero times on the first `set_value` call, but twice on the second `set_value` call. To more precisely define the `set_value` expectations, this commit uses a `Cell`-based implementation to test callbacks.
Instead of encouraging `()` as an Err type, we'd prefer to encourage informative errors. In this case, there is only one failure mode (so we do not need a separate error type) but there is a parameter (exactly which cell is invalid) so we'll keep Result to designate the invalid cell. As discussed in #444 A subtask of #462
Now that it's simplified to use Option, we can simply map into the `get_mut`.
petertseng
changed the title
petertseng
changed the title
|
No need to review this one, this one'll be used as reference for how I rebase whichever one gets merged second. |
petertseng
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #462.
Closes #464 by mutual exclusion.
Note to reviewers: Since this will inevitably conflict with #434, I chose to make all commits on top of #434. This assumes that #434 is getting merged before this one.
You should ignore the commit from #434 in your review of this PR.
If you would instead prefer the changes described in this PR be merged before #434 (perhaps you can review this one very quickly but require more time to review #434), you must instead review #464. Then #434 will rebased on #464.
I have no particular preference on whether the five commits comprising this PR should be squashed (but please do not squash them together with #434).