From 87b73c9529fe24045b0653ec465545492b709e4e Mon Sep 17 00:00:00 2001 From: Peter Tseng Date: Fri, 16 Mar 2018 01:06:39 +0000 Subject: [PATCH 1/5] react: return erroneous CellID from create_compute 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 https://github.com/exercism/rust/issues/444 A subtask of https://github.com/exercism/rust/issues/462 --- exercises/react/example.rs | 6 +++--- exercises/react/src/lib.rs | 6 ++++-- exercises/react/tests/react.rs | 4 ++-- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/exercises/react/example.rs b/exercises/react/example.rs index f3ac5c60c..e649fe4d4 100644 --- a/exercises/react/example.rs +++ b/exercises/react/example.rs @@ -46,11 +46,11 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> { self.cells.len() - 1 } - pub fn create_compute T + 'a>(&mut self, dependencies: &[CellID], compute_func: F) -> Result { + pub fn create_compute T + 'a>(&mut self, dependencies: &[CellID], compute_func: F) -> Result { // 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()) { + return Err(invalid); } let new_id = self.cells.len(); for &id in dependencies { diff --git a/exercises/react/src/lib.rs b/exercises/react/src/lib.rs index 4ec956768..b2fd952c7 100644 --- a/exercises/react/src/lib.rs +++ b/exercises/react/src/lib.rs @@ -28,12 +28,14 @@ impl Reactor { // You do not need to reject compute functions that expect more arguments than there are // dependencies (how would you check for this, anyway?). // - // Return an Err (and you can change the error type) if any dependency doesn't exist. + // If any dependency doesn't exist, returns an Err with that nonexistent dependency. + // (If multiple dependencies do not exist, exactly which one is returned is not defined and + // will not be tested) // // Notice that there is no way to *remove* a cell. // This means that you may assume, without checking, that if the dependencies exist at creation // time they will continue to exist as long as the Reactor exists. - pub fn create_compute T>(&mut self, dependencies: &[CellID], compute_func: F) -> Result { + pub fn create_compute T>(&mut self, dependencies: &[CellID], compute_func: F) -> Result { unimplemented!() } diff --git a/exercises/react/tests/react.rs b/exercises/react/tests/react.rs index c4af9c775..db5780436 100644 --- a/exercises/react/tests/react.rs +++ b/exercises/react/tests/react.rs @@ -50,7 +50,7 @@ fn compute_cells_take_inputs_in_the_right_order() { fn error_creating_compute_cell_if_input_doesnt_exist() { let mut dummy_reactor = Reactor::new(); let input = dummy_reactor.create_input(1); - assert!(Reactor::new().create_compute(&[input], |_| 0).is_err()); + assert_eq!(Reactor::new().create_compute(&[input], |_| 0), Err(input)); } #[test] @@ -61,7 +61,7 @@ fn do_not_break_cell_if_creating_compute_cell_with_valid_and_invalid_input() { let dummy_cell = dummy_reactor.create_input(2); let mut reactor = Reactor::new(); let input = reactor.create_input(1); - assert!(reactor.create_compute(&[input, dummy_cell], |_| 0).is_err()); + assert_eq!(reactor.create_compute(&[input, dummy_cell], |_| 0), Err(dummy_cell)); assert!(reactor.set_value(input, 5).is_ok()); assert_eq!(reactor.value(input), Some(5)); } From cb5a116889fc6a39e5a12a0fd3ab9876e9fd9459 Mon Sep 17 00:00:00 2001 From: Peter Tseng Date: Fri, 16 Mar 2018 01:13:32 +0000 Subject: [PATCH 2/5] react: return Option from add_callback Instead of encouraging `()` as an Err type, we'd prefer to encourage informative errors. In this case, there is only one failure mode and it requires no extra information (there is only one cell specified) so Option is sufficient. As discussed in https://github.com/exercism/rust/issues/444 A subtask of https://github.com/exercism/rust/issues/462 --- exercises/react/example.rs | 6 +++--- exercises/react/src/lib.rs | 4 ++-- exercises/react/tests/react.rs | 16 ++++++++-------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/exercises/react/example.rs b/exercises/react/example.rs index e649fe4d4..4b3cb2d6b 100644 --- a/exercises/react/example.rs +++ b/exercises/react/example.rs @@ -88,14 +88,14 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> { }) } - pub fn add_callback () + 'a>(&mut self, id: CellID, callback: F) -> Result { + pub fn add_callback () + 'a>(&mut self, id: CellID, callback: F) -> Option { match self.cells.get_mut(id) { Some(c) => { c.callbacks_issued += 1; c.callbacks.insert(c.callbacks_issued, Box::new(callback)); - Ok(c.callbacks_issued) + Some(c.callbacks_issued) }, - None => Err("Can't add callback to nonexistent cell"), + None => None, } } diff --git a/exercises/react/src/lib.rs b/exercises/react/src/lib.rs index b2fd952c7..47080d9d6 100644 --- a/exercises/react/src/lib.rs +++ b/exercises/react/src/lib.rs @@ -65,7 +65,7 @@ impl Reactor { // Adds a callback to the specified compute cell. // - // Return an Err (and you can change the error type) if the cell does not exist. + // Returns the ID of the just-added callback, or None if the cell doesn't exist. // // Callbacks on input cells will not be tested. // @@ -75,7 +75,7 @@ impl Reactor { // * Exactly once if the compute cell's value changed as a result of the set_value call. // The value passed to the callback should be the final value of the compute cell after the // set_value call. - pub fn add_callback ()>(&mut self, id: CellID, callback: F) -> Result { + pub fn add_callback ()>(&mut self, id: CellID, callback: F) -> Option { unimplemented!() } diff --git a/exercises/react/tests/react.rs b/exercises/react/tests/react.rs index db5780436..9138a6553 100644 --- a/exercises/react/tests/react.rs +++ b/exercises/react/tests/react.rs @@ -139,7 +139,7 @@ fn compute_cells_fire_callbacks() { let mut reactor = Reactor::new(); let input = reactor.create_input(1); let output = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); - assert!(reactor.add_callback(output, |v| cb.callback_called(v)).is_ok()); + assert!(reactor.add_callback(output, |v| cb.callback_called(v)).is_some()); assert!(reactor.set_value(input, 3).is_ok()); cb.expect_to_have_been_called_with(4); } @@ -150,7 +150,7 @@ fn error_adding_callback_to_nonexistent_cell() { let mut dummy_reactor = Reactor::new(); let input = dummy_reactor.create_input(1); let output = dummy_reactor.create_compute(&[input], |_| 0).unwrap(); - assert!(Reactor::new().add_callback(output, |_: usize| println!("hi")).is_err()); + assert_eq!(Reactor::new().add_callback(output, |_: usize| println!("hi")), None); } #[test] @@ -160,7 +160,7 @@ fn callbacks_only_fire_on_change() { let mut reactor = Reactor::new(); let input = reactor.create_input(1); let output = reactor.create_compute(&[input], |v| if v[0] < 3 { 111 } else { 222 }).unwrap(); - assert!(reactor.add_callback(output, |v| cb.callback_called(v)).is_ok()); + assert!(reactor.add_callback(output, |v| cb.callback_called(v)).is_some()); assert!(reactor.set_value(input, 2).is_ok()); cb.expect_not_to_have_been_called(); @@ -180,14 +180,14 @@ fn callbacks_can_be_added_and_removed() { 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_ok()); + assert!(reactor.add_callback(output, |v| cb2.callback_called(v)).is_some()); assert!(reactor.set_value(input, 31).is_ok()); cb1.expect_to_have_been_called_with(32); cb2.expect_to_have_been_called_with(32); assert!(reactor.remove_callback(output, callback).is_ok()); - assert!(reactor.add_callback(output, |v| cb3.callback_called(v)).is_ok()); + assert!(reactor.add_callback(output, |v| cb3.callback_called(v)).is_some()); assert!(reactor.set_value(input, 41).is_ok()); cb1.expect_not_to_have_been_called(); @@ -205,7 +205,7 @@ fn removing_a_callback_multiple_times_doesnt_interfere_with_other_callbacks() { let input = reactor.create_input(1); 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_ok()); + 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. assert!(reactor.remove_callback(output, callback).is_ok()); for _ in 1..5 { @@ -227,7 +227,7 @@ fn callbacks_should_only_be_called_once_even_if_multiple_dependencies_change() { let minus_one1 = reactor.create_compute(&[input], |v| v[0] - 1).unwrap(); let minus_one2 = reactor.create_compute(&[minus_one1], |v| v[0] - 1).unwrap(); let output = reactor.create_compute(&[plus_one, minus_one2], |v| v[0] * v[1]).unwrap(); - assert!(reactor.add_callback(output, |v| cb.callback_called(v)).is_ok()); + assert!(reactor.add_callback(output, |v| cb.callback_called(v)).is_some()); assert!(reactor.set_value(input, 4).is_ok()); cb.expect_to_have_been_called_with(10); } @@ -241,7 +241,7 @@ fn callbacks_should_not_be_called_if_dependencies_change_but_output_value_doesnt let plus_one = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); let minus_one = reactor.create_compute(&[input], |v| v[0] - 1).unwrap(); let always_two = reactor.create_compute(&[plus_one, minus_one], |v| v[0] - v[1]).unwrap(); - assert!(reactor.add_callback(always_two, |v| cb.callback_called(v)).is_ok()); + assert!(reactor.add_callback(always_two, |v| cb.callback_called(v)).is_some()); for i in 2..5 { assert!(reactor.set_value(input, i).is_ok()); cb.expect_not_to_have_been_called(); From f252e6fc9ea53292d80054ea4ccbdfab6a6b8c3d Mon Sep 17 00:00:00 2001 From: Peter Tseng Date: Fri, 16 Mar 2018 01:14:50 +0000 Subject: [PATCH 3/5] react: Use Option::map in add_callback example Now that it's simplified to use Option, we can simply map into the `get_mut`. --- exercises/react/example.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/exercises/react/example.rs b/exercises/react/example.rs index 4b3cb2d6b..c07a7f6bb 100644 --- a/exercises/react/example.rs +++ b/exercises/react/example.rs @@ -89,14 +89,11 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> { } pub fn add_callback () + 'a>(&mut self, id: CellID, callback: F) -> Option { - match self.cells.get_mut(id) { - Some(c) => { - c.callbacks_issued += 1; - c.callbacks.insert(c.callbacks_issued, Box::new(callback)); - Some(c.callbacks_issued) - }, - None => None, - } + self.cells.get_mut(id).map(|c| { + c.callbacks_issued += 1; + c.callbacks.insert(c.callbacks_issued, Box::new(callback)); + c.callbacks_issued + }) } pub fn remove_callback(&mut self, cell: CellID, callback: CallbackID) -> Result<(), &'static str> { From 0122044210b2af93c99c7a7cc0755450aa337b2e Mon Sep 17 00:00:00 2001 From: Peter Tseng Date: Fri, 16 Mar 2018 01:21:28 +0000 Subject: [PATCH 4/5] react: return domain-specific error from set_value Instead of encouraging `()` as an Err type, we'd prefer to encourage informative errors. In this case, there are multiple failure modes so we need to have a specific enum for it. As discussed in https://github.com/exercism/rust/issues/444 A subtask of https://github.com/exercism/rust/issues/462 --- exercises/react/example.rs | 12 +++++++++--- exercises/react/src/lib.rs | 14 +++++++++++--- exercises/react/tests/react.rs | 4 ++-- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/exercises/react/example.rs b/exercises/react/example.rs index c07a7f6bb..e533c0a81 100644 --- a/exercises/react/example.rs +++ b/exercises/react/example.rs @@ -3,6 +3,12 @@ use std::collections::HashMap; pub type CellID = usize; pub type CallbackID = usize; +#[derive(Debug, PartialEq)] +pub enum SetValueError { + NonexistentCell, + ComputeCell, +} + struct Cell<'a, T: Copy> { value: T, last_value: T, @@ -66,16 +72,16 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> { self.cells.get(id).map(|c| c.value) } - pub fn set_value(&mut self, id: CellID, new_value: T) -> Result<(), &'static str> { + pub fn set_value(&mut self, id: CellID, new_value: T) -> Result<(), SetValueError> { match self.cells.get_mut(id) { Some(c) => match c.cell_type { CellType::Input => { c.value = new_value; Ok(c.dependents.clone()) }, - CellType::Compute(_, _) => Err("Can't set compute cell value directly"), + CellType::Compute(_, _) => Err(SetValueError::ComputeCell), }, - None => Err("Can't set nonexistent cell"), + None => Err(SetValueError::NonexistentCell), }.map(|deps| { for &d in deps.iter() { self.update_dependent(d); diff --git a/exercises/react/src/lib.rs b/exercises/react/src/lib.rs index 47080d9d6..8c6bf3a24 100644 --- a/exercises/react/src/lib.rs +++ b/exercises/react/src/lib.rs @@ -5,6 +5,12 @@ pub type CellID = (); pub type CallbackID = (); +#[derive(Debug, PartialEq)] +pub enum SetValueError { + NonexistentCell, + ComputeCell, +} + pub struct Reactor { // Just so that the compiler doesn't complain about an unused type parameter. // You probably want to delete this field. @@ -52,14 +58,16 @@ impl Reactor { // Sets the value of the specified input cell. // - // Return an Err (and you can change the error type) if the cell does not exist, or the - // specified cell is a compute cell, since compute cells cannot have their values directly set. + // Returns an Err if either: + // * the cell does not exist + // * the specified cell is a compute cell, since compute cells cannot have their values + // directly set. // // Similarly, you may wonder about `get_mut(&mut self, id: CellID) -> Option<&mut Cell>`, with // a `set_value(&mut self, new_value: T)` method on `Cell`. // // As before, that turned out to add too much extra complexity. - pub fn set_value(&mut self, id: CellID, new_value: T) -> Result<(), ()> { + pub fn set_value(&mut self, id: CellID, new_value: T) -> Result<(), SetValueError> { unimplemented!() } diff --git a/exercises/react/tests/react.rs b/exercises/react/tests/react.rs index 9138a6553..1606c09c6 100644 --- a/exercises/react/tests/react.rs +++ b/exercises/react/tests/react.rs @@ -23,7 +23,7 @@ fn an_input_cells_value_can_be_set() { fn error_setting_a_nonexistent_input_cell() { let mut dummy_reactor = Reactor::new(); let input = dummy_reactor.create_input(1); - assert!(Reactor::new().set_value(input, 0).is_err()); + assert_eq!(Reactor::new().set_value(input, 0), Err(SetValueError::NonexistentCell)); } #[test] @@ -96,7 +96,7 @@ fn error_setting_a_compute_cell() { let mut reactor = Reactor::new(); let input = reactor.create_input(1); let output = reactor.create_compute(&[input], |_| 0).unwrap(); - assert!(reactor.set_value(output, 3).is_err()); + assert_eq!(reactor.set_value(output, 3), Err(SetValueError::ComputeCell)); } /// A CallbackRecorder helps tests whether callbacks get called correctly. From 36354b440ea1b9bc162ec58e21e3cb843fd05f14 Mon Sep 17 00:00:00 2001 From: Peter Tseng Date: Fri, 16 Mar 2018 01:41:34 +0000 Subject: [PATCH 5/5] react: return domain-specific error from remove_callback Instead of encouraging `()` as an Err type, we'd prefer to encourage informative errors. In this case, there are multiple failure modes so we need to have a specific enum for it. As discussed in https://github.com/exercism/rust/issues/444 A subtask of https://github.com/exercism/rust/issues/462 --- exercises/react/example.rs | 12 +++++++++--- exercises/react/src/lib.rs | 11 ++++++++--- exercises/react/tests/react.rs | 4 ++-- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/exercises/react/example.rs b/exercises/react/example.rs index e533c0a81..096426960 100644 --- a/exercises/react/example.rs +++ b/exercises/react/example.rs @@ -9,6 +9,12 @@ pub enum SetValueError { ComputeCell, } +#[derive(Debug, PartialEq)] +pub enum RemoveCallbackError { + NonexistentCell, + NonexistentCallback, +} + struct Cell<'a, T: Copy> { value: T, last_value: T, @@ -102,13 +108,13 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> { }) } - pub fn remove_callback(&mut self, cell: CellID, callback: CallbackID) -> Result<(), &'static str> { + pub fn remove_callback(&mut self, cell: CellID, callback: CallbackID) -> Result<(), RemoveCallbackError> { match self.cells.get_mut(cell) { Some(c) => match c.callbacks.remove(&callback) { Some(_) => Ok(()), - None => Err("Can't remove nonexistent callback"), + None => Err(RemoveCallbackError::NonexistentCallback), }, - None => Err("Can't remove callback from nonexistent cell"), + None => Err(RemoveCallbackError::NonexistentCell), } } diff --git a/exercises/react/src/lib.rs b/exercises/react/src/lib.rs index 8c6bf3a24..600840ad2 100644 --- a/exercises/react/src/lib.rs +++ b/exercises/react/src/lib.rs @@ -11,6 +11,12 @@ pub enum SetValueError { ComputeCell, } +#[derive(Debug, PartialEq)] +pub enum RemoveCallbackError { + NonexistentCell, + NonexistentCallback, +} + pub struct Reactor { // Just so that the compiler doesn't complain about an unused type parameter. // You probably want to delete this field. @@ -89,11 +95,10 @@ impl Reactor { // Removes the specified callback, using an ID returned from add_callback. // - // Return an Err (and you can change the error type) if either the cell or callback - // does not exist. + // Returns an Err if either the cell or callback does not exist. // // A removed callback should no longer be called. - pub fn remove_callback(&mut self, cell: CellID, callback: CallbackID) -> Result<(), ()> { + pub fn remove_callback(&mut self, cell: CellID, callback: CallbackID) -> Result<(), RemoveCallbackError> { unimplemented!() } } diff --git a/exercises/react/tests/react.rs b/exercises/react/tests/react.rs index 1606c09c6..95ab71613 100644 --- a/exercises/react/tests/react.rs +++ b/exercises/react/tests/react.rs @@ -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. assert!(reactor.remove_callback(output, callback).is_ok()); for _ in 1..5 { - assert!(reactor.remove_callback(output, callback).is_err()); + assert_eq!(reactor.remove_callback(output, callback), Err(RemoveCallbackError::NonexistentCallback)); } assert!(reactor.set_value(input, 2).is_ok());