fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

refactor: replace CellValue::Empty with Option<CellValue>

Browse Source 
Previously CellValue had three variants: Number, Text, and Empty.
The Empty variant acted as a null sentinel, but the compiler could not
distinguish between "this is a real value" and "this might be empty".
Code that received a CellValue could use it without checking for Empty,
because there was no type-level enforcement.

Now CellValue has only Number and Text. The absence of a value is
represented as None at every API boundary:

  DataStore::get()    → Option<&CellValue>  (was &CellValue / Empty)
  Model::get_cell()   → Option<&CellValue>  (was &CellValue / Empty)
  Model::evaluate()   → Option<CellValue>   (was CellValue::Empty)
  eval_formula()      → Option<CellValue>   (was CellValue::Empty)

Model gains clear_cell() for explicit key removal; ClearCell dispatch
calls it instead of set_cell(key, CellValue::Empty).

The compiler now forces every caller of evaluate/get_cell to handle
the None case explicitly — accidental use of an empty value as if it
were real is caught at compile time rather than silently computing
wrong results.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Ed L
2026-03-24 08:06:51 -07:00
parent e680b098ec
commit a2e519efcc
8 changed files with 95 additions and 109 deletions
Download Patch File Download Diff File Expand all files Collapse all files

48
src/model/cell.rs
View File

@ -48,20 +48,15 @@ impl std::fmt::Display for CellKey {
pub enum CellValue {
Number(f64),
Text(String),
Empty,
}
impl CellValue {
pub fn as_f64(&self) -> Option<f64> {
match self {
CellValue::Number(n) => Some(*n),
_ => None,
CellValue::Text(_) => None,
}
}
pub fn is_empty(&self) -> bool {
matches!(self, CellValue::Empty)
}
}
impl std::fmt::Display for CellValue {
@ -75,17 +70,10 @@ impl std::fmt::Display for CellValue {
}
}
CellValue::Text(s) => write!(f, "{s}"),
CellValue::Empty => write!(f, ""),
}
}
}
impl Default for CellValue {
fn default() -> Self {
CellValue::Empty
}
}
/// Serialized as a list of (key, value) pairs so CellKey doesn't need
/// to implement the `Serialize`-as-string requirement for JSON object keys.
#[derive(Debug, Clone, Default)]
@ -118,15 +106,11 @@ impl DataStore {
}
pub fn set(&mut self, key: CellKey, value: CellValue) {
if value.is_empty() {
self.cells.remove(&key);
} else {
self.cells.insert(key, value);
}
self.cells.insert(key, value);
}
pub fn get(&self, key: &CellKey) -> &CellValue {
self.cells.get(key).unwrap_or(&CellValue::Empty)
pub fn get(&self, key: &CellKey) -> Option<&CellValue> {
self.cells.get(key)
}
pub fn get_mut(&mut self, key: &CellKey) -> Option<&mut CellValue> {
@ -261,7 +245,7 @@ mod data_store {
#[test]
fn get_missing_returns_empty() {
let store = DataStore::new();
assert_eq!(store.get(&key(&[("Region", "East")])), &CellValue::Empty);
assert_eq!(store.get(&key(&[("Region", "East")])), None);
}
#[test]
@ -269,7 +253,7 @@ mod data_store {
let mut store = DataStore::new();
let k = key(&[("Region", "East"), ("Product", "Shirts")]);
store.set(k.clone(), CellValue::Number(42.0));
assert_eq!(store.get(&k), &CellValue::Number(42.0));
assert_eq!(store.get(&k), Some(&CellValue::Number(42.0)));
}
#[test]
@ -278,15 +262,15 @@ mod data_store {
let k = key(&[("Region", "East")]);
store.set(k.clone(), CellValue::Number(1.0));
store.set(k.clone(), CellValue::Number(99.0));
assert_eq!(store.get(&k), &CellValue::Number(99.0));
assert_eq!(store.get(&k), Some(&CellValue::Number(99.0)));
}
#[test]
fn setting_empty_removes_key() {
fn remove_evicts_key() {
let mut store = DataStore::new();
let k = key(&[("Region", "East")]);
store.set(k.clone(), CellValue::Number(5.0));
store.set(k.clone(), CellValue::Empty);
store.remove(&k);
assert!(store.cells().is_empty());
}
@ -433,17 +417,17 @@ mod prop_tests {
let key = CellKey::new(pairs);
let mut store = DataStore::default();
store.set(key.clone(), CellValue::Number(val));
prop_assert_eq!(store.get(&key), &CellValue::Number(val));
prop_assert_eq!(store.get(&key), Some(&CellValue::Number(val)));
}
/// Setting Empty after a real value: get returns Empty (key is evicted).
/// Removing after a real value: get returns None (key is evicted).
#[test]
fn datastore_empty_evicts_key(pairs in pairs_map(), val in finite_f64()) {
let key = CellKey::new(pairs);
let mut store = DataStore::default();
store.set(key.clone(), CellValue::Number(val));
store.set(key.clone(), CellValue::Empty);
prop_assert_eq!(store.get(&key), &CellValue::Empty);
store.remove(&key);
prop_assert_eq!(store.get(&key), None);
}
/// The last write to a key wins.
@ -457,7 +441,7 @@ mod prop_tests {
let mut store = DataStore::default();
store.set(key.clone(), CellValue::Number(v1));
store.set(key.clone(), CellValue::Number(v2));
prop_assert_eq!(store.get(&key), &CellValue::Number(v2));
prop_assert_eq!(store.get(&key), Some(&CellValue::Number(v2)));
}
/// Two keys that differ by one coordinate are fully independent.
@ -480,9 +464,9 @@ mod prop_tests {
let mut store = DataStore::default();
store.set(key1.clone(), CellValue::Number(v1));
store.set(key2.clone(), CellValue::Number(v2));
prop_assert_eq!(store.get(&key1), &CellValue::Number(v1),
prop_assert_eq!(store.get(&key1), Some(&CellValue::Number(v1)),
"key1 corrupted after writing key2 (diff in {})", changed_cat);
prop_assert_eq!(store.get(&key2), &CellValue::Number(v2));
prop_assert_eq!(store.get(&key2), Some(&CellValue::Number(v2)));
}
/// Every cell returned by matching_cells actually satisfies the partial key.