react: return domain-specific errors #464
Merged
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
petertseng
commented
Mar 16, 2018
exercises/react/src/lib.rs
Outdated
| @@ -11,6 +11,12 @@ pub enum SetValueError { | |||
| ComputeCell, | |||
| } | |||
|
|
|||
| #[derive(Debug, PartialEq)] | |||
| pub enum RemoveCallbackError { | |||
| NonexistentCell, | |||
There was a problem hiding this comment.
note: It apparently is the case that we never test the NonexistentCell case. That can be dealt with later.
petertseng
changed the title
petertseng
changed the title
petertseng
commented
Mar 16, 2018
| // Check all dependencies' validity before modifying any of them, | ||
| // so that we don't perform an incorrect partial write. | ||
| if !dependencies.iter().all(|&id| id < self.cells.len()) { | ||
| return Err("Nonexistent input"); | ||
| if let Some(&invalid) = dependencies.iter().find(|&dep| *dep >= self.cells.len()) { |
There was a problem hiding this comment.
There is in fact not a specific reason I wrote find(|&dep| *dep >= self.cells.len()) as opposed to one of these two alternatives:
find(|&&dep| dep >= self.cells.len())
find(|dep| **dep >= self.cells.len())
I really don't know what I'm doing and which is better, so I need help on that.
coriolinus
previously approved these changes
Apr 4, 2018
|
Last reviewed: ff451ea |
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`.
|
This merge was more involved. I'll ask for a re-review on this one to make sure I didn't screw anything up. |
petertseng
dismissed
coriolinus’s stale review
This merge was more involved. I'll ask for a re-review on this one to make sure I didn't screw anything up.
petertseng
commented
Apr 4, 2018
exercises/react/tests/react.rs
Outdated
| @@ -206,10 +206,10 @@ fn removing_a_callback_multiple_times_doesnt_interfere_with_other_callbacks() { | |||
| let output = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); | |||
| let callback = reactor.add_callback(output, |v| cb1.callback_called(v)).unwrap(); | |||
| assert!(reactor.add_callback(output, |v| cb2.callback_called(v)).is_some()); | |||
| // We want the first remove to be Ok, but we don't care about the others. | |||
| // We want the first remove to be Ok, but the others should be errors. | |||
coriolinus
approved these changes
Apr 4, 2018
|
Thanks for that! |
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.
Details are in individual commit messages.
I have no particular preference on whether the five commits comprising this PR should be squashed.