Skip to content

Commit 0f10f71

Browse files
hyperpolymathclaude
andcommitted
refactor(server): close .expect("TODO") anti-pattern
Eliminates the bulk-rewrite anti-pattern (.unwrap() -> .expect("TODO: handle error") substitution that gives identical panic behaviour while pretending to mark debt). 35 sites cleared across 10 Rust files. Production restructures (5 sites, all in working tree before this commit): - core.rs `html_to_markdown`: two `Selector::parse(...).expect()` calls reworked to `if let Ok(selector) = Selector::parse(...)` so a malformed selector is structural, not a panic. Mirrors the pattern already used elsewhere in the same function. (policy a/c-style, structural pivot.) - core.rs `html_to_markdown` body fallback: `if let Some(body) ... else` over a `Selector::parse("body").expect()` rewritten to `match ... .ok().and_then(...)` so both the parse failure and empty selection cleanly fall back to root text. - core.rs `html_to_json` heading loop: same `Selector::parse` if-let pivot. - monitoring.rs `snapshot`: `if !durations.is_empty() { ... durations.iter() .max().expect() ... .min().expect() ... }` collapsed into a single `iter().copied().fold(None, ...)` with `let-else` continue, removing the `.expect()` pair entirely. (policy b, structural restructure on Option.) Test sites (30 sites, mass `replace_all` to `.unwrap()` after verifying every call sat below the file's `#[cfg(test)]` line): auth.rs (1), core.rs (7), document_store.rs (3), formats/mod.rs (4), formats/toml.rs (1), formats/xml.rs (2), formats/yaml.rs (2), http.rs (7), websocket.rs (3). Test counts unchanged: 152 passed / 0 failed / 0 ignored before and after. `cargo build` clean (pre-existing unused-variable warnings only). Pattern proven on valence-shell (~512 sites) and 3 estate-batch-1 peers (reposystem 33, vexometer 44, double-track-browser 47). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent a645db8 commit 0f10f71

10 files changed

Lines changed: 84 additions & 70 deletions

File tree

server/src/auth.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ mod tests {
296296
let config = AuthConfig::default();
297297
let service = AuthService::new(config);
298298

299-
let token = service.generate_token("user123".to_string(), vec!["read".to_string()]).expect("TODO: handle error");
299+
let token = service.generate_token("user123".to_string(), vec!["read".to_string()]).unwrap();
300300
assert!(token.starts_with("Bearer "));
301301
}
302302

server/src/core.rs

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -227,10 +227,13 @@ impl ConversionCore {
227227

228228
// Extract headings
229229
for level in 1..=6 {
230-
let selector = Selector::parse(&format!("h{level}")).expect("TODO: handle error");
231-
for element in document.select(&selector) {
232-
let text = element.text().collect::<String>();
233-
markdown.push_str(&format!("{} {}\n\n", "#".repeat(level), text.trim()));
230+
// Mirror the `if let Ok(selector) = Selector::parse(...)` pattern used below
231+
// for the other selectors so a parse failure is structural, not a panic.
232+
if let Ok(selector) = Selector::parse(&format!("h{level}")) {
233+
for element in document.select(&selector) {
234+
let text = element.text().collect::<String>();
235+
markdown.push_str(&format!("{} {}\n\n", "#".repeat(level), text.trim()));
236+
}
234237
}
235238
}
236239

@@ -266,13 +269,17 @@ impl ConversionCore {
266269
}
267270
}
268271

269-
// If we got nothing, just extract all text
272+
// If we got nothing, just extract all text. Use if-let on the selector parse so
273+
// a malformed selector falls back to root-element text rather than panicking.
270274
if markdown.trim().is_empty() {
271-
let body_selector = Selector::parse("body").expect("TODO: handle error");
272-
if let Some(body) = document.select(&body_selector).next() {
273-
markdown = body.text().collect::<String>();
274-
} else {
275-
markdown = document.root_element().text().collect::<String>();
275+
match Selector::parse("body").ok().and_then(|sel| {
276+
document
277+
.select(&sel)
278+
.next()
279+
.map(|body| body.text().collect::<String>())
280+
}) {
281+
Some(body_text) => markdown = body_text,
282+
None => markdown = document.root_element().text().collect::<String>(),
276283
}
277284
}
278285

@@ -297,12 +304,15 @@ impl ConversionCore {
297304
let text: String = document.root_element().text().collect();
298305
data.insert("content", text.trim().to_string());
299306

300-
// Extract headings
307+
// Extract headings — same if-let pattern as html_to_markdown above to avoid panics
308+
// on a malformed selector (in practice "h1".."h6" always parse, but stay structural).
301309
let mut headings = Vec::new();
302310
for level in 1..=6 {
303-
let selector = Selector::parse(&format!("h{level}")).expect("TODO: handle error");
304-
for element in document.select(&selector) {
305-
headings.push(format!("H{}: {}", level, element.text().collect::<String>()));
311+
if let Ok(selector) = Selector::parse(&format!("h{level}")) {
312+
for element in document.select(&selector) {
313+
headings
314+
.push(format!("H{}: {}", level, element.text().collect::<String>()));
315+
}
306316
}
307317
}
308318
if !headings.is_empty() {
@@ -412,7 +422,7 @@ mod tests {
412422
#[test]
413423
fn test_html_to_markdown() {
414424
let html = "<h1>Hello World</h1><p>This is a test.</p>";
415-
let markdown = ConversionCore::html_to_markdown(html).expect("TODO: handle error");
425+
let markdown = ConversionCore::html_to_markdown(html).unwrap();
416426
assert!(markdown.contains("# Hello World"));
417427
assert!(markdown.contains("This is a test"));
418428
}
@@ -424,7 +434,7 @@ mod tests {
424434
from: Format::Markdown,
425435
to: Format::Html,
426436
};
427-
let response = ConversionCore::convert(request).expect("TODO: handle error");
437+
let response = ConversionCore::convert(request).unwrap();
428438
assert!(response.content.contains("<h1>"));
429439
}
430440

@@ -435,7 +445,7 @@ mod tests {
435445
from: Format::Markdown,
436446
to: Format::Markdown,
437447
};
438-
let response = ConversionCore::convert(request).expect("TODO: handle error");
448+
let response = ConversionCore::convert(request).unwrap();
439449
assert_eq!(response.content, "# Test");
440450
}
441451

@@ -447,25 +457,25 @@ mod tests {
447457
from: Format::Markdown,
448458
to: Format::Json,
449459
};
450-
let json_response = ConversionCore::convert(request).expect("TODO: handle error");
460+
let json_response = ConversionCore::convert(request).unwrap();
451461

452462
let request = ConversionRequest {
453463
content: json_response.content,
454464
from: Format::Json,
455465
to: Format::Markdown,
456466
};
457-
let md_response = ConversionCore::convert(request).expect("TODO: handle error");
467+
let md_response = ConversionCore::convert(request).unwrap();
458468
assert!(md_response.content.contains("Title"));
459469
}
460470

461471
#[test]
462472
fn test_validate_json() {
463473
let valid = r#"{"key": "value"}"#;
464-
let diagnostics = ConversionCore::validate(valid, Format::Json).expect("TODO: handle error");
474+
let diagnostics = ConversionCore::validate(valid, Format::Json).unwrap();
465475
assert!(diagnostics.is_empty());
466476

467477
let invalid = r#"{"key": invalid}"#;
468-
let diagnostics = ConversionCore::validate(invalid, Format::Json).expect("TODO: handle error");
478+
let diagnostics = ConversionCore::validate(invalid, Format::Json).unwrap();
469479
assert!(!diagnostics.is_empty());
470480
}
471481
}

server/src/document_store.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ mod tests {
194194
assert_eq!(store.count(), 1);
195195

196196
// Get
197-
let doc = store.get("file:///test.md").expect("TODO: handle error");
197+
let doc = store.get("file:///test.md").unwrap();
198198
assert_eq!(doc.content, "# Hello");
199199

200200
// Update
@@ -203,7 +203,7 @@ mod tests {
203203
"# Hello World".to_string(),
204204
"markdown".to_string(),
205205
);
206-
let doc = store.get("file:///test.md").expect("TODO: handle error");
206+
let doc = store.get("file:///test.md").unwrap();
207207
assert_eq!(doc.content, "# Hello World");
208208
assert_eq!(doc.version, 2);
209209

@@ -237,7 +237,7 @@ mod tests {
237237
}
238238

239239
for handle in handles {
240-
handle.join().expect("TODO: handle error");
240+
handle.join().unwrap();
241241
}
242242

243243
assert_eq!(store.count(), 1000);

server/src/formats/mod.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,10 @@ mod tests {
5252

5353
#[test]
5454
fn test_format_from_str() {
55-
assert_eq!(ExtendedFormat::from_str("yaml").expect("TODO: handle error"), ExtendedFormat::Yaml);
56-
assert_eq!(ExtendedFormat::from_str("yml").expect("TODO: handle error"), ExtendedFormat::Yaml);
57-
assert_eq!(ExtendedFormat::from_str("xml").expect("TODO: handle error"), ExtendedFormat::Xml);
58-
assert_eq!(ExtendedFormat::from_str("toml").expect("TODO: handle error"), ExtendedFormat::Toml);
55+
assert_eq!(ExtendedFormat::from_str("yaml").unwrap(), ExtendedFormat::Yaml);
56+
assert_eq!(ExtendedFormat::from_str("yml").unwrap(), ExtendedFormat::Yaml);
57+
assert_eq!(ExtendedFormat::from_str("xml").unwrap(), ExtendedFormat::Xml);
58+
assert_eq!(ExtendedFormat::from_str("toml").unwrap(), ExtendedFormat::Toml);
5959
}
6060

6161
#[test]

server/src/formats/toml.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ mod tests {
4646

4747
#[test]
4848
fn test_validate_toml_empty() {
49-
let diagnostics = validate_toml("").expect("TODO: handle error");
49+
let diagnostics = validate_toml("").unwrap();
5050
assert!(!diagnostics.is_empty());
5151
}
5252
}

server/src/formats/xml.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,14 @@ mod tests {
4949

5050
#[test]
5151
fn test_validate_xml_empty() {
52-
let diagnostics = validate_xml("").expect("TODO: handle error");
52+
let diagnostics = validate_xml("").unwrap();
5353
assert!(!diagnostics.is_empty());
5454
}
5555

5656
#[test]
5757
fn test_validate_xml_no_declaration() {
5858
let xml = "<root></root>";
59-
let diagnostics = validate_xml(xml).expect("TODO: handle error");
59+
let diagnostics = validate_xml(xml).unwrap();
6060
assert!(diagnostics.iter().any(|d| d.contains("declaration")));
6161
}
6262
}

server/src/formats/yaml.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,14 +65,14 @@ mod tests {
6565
#[test]
6666
fn test_validate_yaml_tabs() {
6767
let yaml = "key:\tvalue"; // Has tab
68-
let diagnostics = validate_yaml(yaml).expect("TODO: handle error");
68+
let diagnostics = validate_yaml(yaml).unwrap();
6969
assert!(!diagnostics.is_empty());
7070
assert!(diagnostics[0].contains("tabs"));
7171
}
7272

7373
#[test]
7474
fn test_validate_yaml_empty() {
75-
let diagnostics = validate_yaml(" ").expect("TODO: handle error");
75+
let diagnostics = validate_yaml(" ").unwrap();
7676
assert!(!diagnostics.is_empty());
7777
}
7878
}

server/src/http.rs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -261,10 +261,10 @@ mod tests {
261261
Request::builder()
262262
.uri("/api/health")
263263
.body(Body::empty())
264-
.expect("TODO: handle error"),
264+
.unwrap(),
265265
)
266266
.await
267-
.expect("TODO: handle error");
267+
.unwrap();
268268

269269
assert_eq!(response.status(), StatusCode::OK);
270270
}
@@ -286,11 +286,11 @@ mod tests {
286286
.method("POST")
287287
.uri("/api/convert")
288288
.header("content-type", "application/json")
289-
.body(Body::from(serde_json::to_string(&payload).expect("TODO: handle error")))
290-
.expect("TODO: handle error"),
289+
.body(Body::from(serde_json::to_string(&payload).unwrap()))
290+
.unwrap(),
291291
)
292292
.await
293-
.expect("TODO: handle error");
293+
.unwrap();
294294

295295
assert_eq!(response.status(), StatusCode::OK);
296296
}
@@ -313,10 +313,10 @@ mod tests {
313313
Request::builder()
314314
.uri("/api/documents")
315315
.body(Body::empty())
316-
.expect("TODO: handle error"),
316+
.unwrap(),
317317
)
318318
.await
319-
.expect("TODO: handle error");
319+
.unwrap();
320320

321321
assert_eq!(response.status(), StatusCode::OK);
322322
}

server/src/monitoring.rs

Lines changed: 30 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -67,32 +67,36 @@ impl Metrics {
6767

6868
for entry in self.request_durations.iter() {
6969
let durations = entry.value();
70-
if !durations.is_empty() {
71-
let sum: u64 = durations.iter().sum();
72-
let avg = sum / durations.len() as u64;
73-
let max = *durations.iter().max().expect("TODO: handle error");
74-
let min = *durations.iter().min().expect("TODO: handle error");
75-
76-
// Calculate percentiles
77-
let mut sorted = durations.clone();
78-
sorted.sort_unstable();
79-
let p50 = sorted[sorted.len() / 2];
80-
let p95 = sorted[sorted.len() * 95 / 100];
81-
let p99 = sorted[sorted.len() * 99 / 100];
82-
83-
endpoint_stats.insert(
84-
entry.key().clone(),
85-
EndpointStats {
86-
count: durations.len() as u64,
87-
avg_ms: avg,
88-
min_ms: min,
89-
max_ms: max,
90-
p50_ms: p50,
91-
p95_ms: p95,
92-
p99_ms: p99,
93-
},
94-
);
95-
}
70+
// Skip empty buckets; otherwise destructure min/max via a single fold so the
71+
// None branch is structural rather than a panic-with-message.
72+
let Some((min, max)) = durations.iter().copied().fold(None, |acc, d| match acc {
73+
None => Some((d, d)),
74+
Some((lo, hi)) => Some((lo.min(d), hi.max(d))),
75+
}) else {
76+
continue;
77+
};
78+
let sum: u64 = durations.iter().sum();
79+
let avg = sum / durations.len() as u64;
80+
81+
// Calculate percentiles
82+
let mut sorted = durations.clone();
83+
sorted.sort_unstable();
84+
let p50 = sorted[sorted.len() / 2];
85+
let p95 = sorted[sorted.len() * 95 / 100];
86+
let p99 = sorted[sorted.len() * 99 / 100];
87+
88+
endpoint_stats.insert(
89+
entry.key().clone(),
90+
EndpointStats {
91+
count: durations.len() as u64,
92+
avg_ms: avg,
93+
min_ms: min,
94+
max_ms: max,
95+
p50_ms: p50,
96+
p95_ms: p95,
97+
p99_ms: p99,
98+
},
99+
);
96100
}
97101

98102
MetricsSnapshot {

server/src/websocket.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -190,8 +190,8 @@ mod tests {
190190
document_id: "doc-123".to_string(),
191191
};
192192

193-
let json = serde_json::to_string(&msg).expect("TODO: handle error");
194-
let deserialized: WsMessage = serde_json::from_str(&json).expect("TODO: handle error");
193+
let json = serde_json::to_string(&msg).unwrap();
194+
let deserialized: WsMessage = serde_json::from_str(&json).unwrap();
195195

196196
match deserialized {
197197
WsMessage::Subscribe { document_id } => {
@@ -209,7 +209,7 @@ mod tests {
209209
timestamp: chrono::Utc::now(),
210210
};
211211

212-
let json = serde_json::to_string(&msg).expect("TODO: handle error");
212+
let json = serde_json::to_string(&msg).unwrap();
213213
assert!(json.contains("DocumentUpdated"));
214214
assert!(json.contains("doc-456"));
215215
}

0 commit comments

Comments
 (0)