refactor(curves,surfaces): eliminate .unwrap()/.expect() (#319)#353
Merged
joaquinbejar merged 5 commits intomainfrom Apr 17, 2026
Merged
refactor(curves,surfaces): eliminate .unwrap()/.expect() (#319)#353joaquinbejar merged 5 commits intomainfrom
joaquinbejar merged 5 commits intomainfrom
Conversation
…xamples (#319) `src/curves/traits.rs` had two `Decimal::to_f64().unwrap()` sites in the statistical-curve regression helper; convert via `ok_or_else` to `CurveError::invalid_parameters`. `src/curves/visualization/{plotters,mod}.rs` and `src/surfaces/visualization/plotters.rs` doc-comment examples used `.expect("panic message")` and `.unwrap_or_else(|_| panic!(...))`; wrap each in a `# fn main() -> Result<(), Box<dyn Error>>` shim and propagate via `?`.
Eliminate every production .unwrap()/.expect() in src/curves/curve.rs.
- Index<usize>::index keeps the trait-mandated panic on out-of-bounds
but uses an explicit panic! with a clear "index = N, len = M"
message instead of .expect("Index out of bounds"); documents in
the function-level # Panics section.
- Decimal::partial_cmp(...).unwrap() in sort/min_by/max_by closures
now falls back to Ordering::Equal per global_rules.md §Numerical
Discipline (NaN guard at f64↔Decimal boundary).
- min_by/max_by Option chains in compute_range_metrics and extrema
surface MetricsError::BasicError / CurveError::AnalysisError on
empty inputs instead of panicking.
- merge_axis_interpolate's get_point unwraps inside contains_point
branches now surface CurveError::InterpolationError; defensively
guards an invariant that should always hold but no longer panics.
Eliminate every production .unwrap()/.expect() in src/surfaces/surface.rs.
- Index<usize>::index keeps the trait-mandated panic on out-of-bounds
but uses an explicit panic! with a clear "index = N, len = M"
message instead of .expect("Index out of bounds"); documents in
the function-level # Panics section.
- Decimal::partial_cmp(...).unwrap() in sort/min_by/max_by closures
now falls back to Ordering::Equal per global_rules.md §Numerical
Discipline.
- min_by/max_by Option chains in extrema and merge_axis_interpolate
now surface SurfaceError::AnalysisError on empty / missing-point
invariants instead of panicking.
- Decimal::sqrt().unwrap() in cubic_interpolate weights and
get_closest_point comparator (which cannot return Result) fall back
to Decimal::ZERO with a tracing::warn! diagnostic; the squared
operands are non-negative so the failure path is theoretical
(overflow only).
- Doc-comment examples in GeometricObject::construct wrapped in a
fn run() -> Result<(), Box<dyn Error>> shim so they no longer use
.unwrap().
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR continues the “Panic-Free Core” milestone work by removing .unwrap() / .expect() from production code in the curves and surfaces modules, replacing them with error propagation, safe fallbacks in comparators, and updated doctest patterns.
Changes:
- Replaces
.unwrap()/.expect()in curve/surface implementations withResult-based error propagation (ok_or_else(...)?) and guarded comparator fallbacks. - Updates
Index<usize>implementations to use explicitpanic!messages (preservingIndex’s contract) and documents panics. - Refactors visualization doctest examples to return
Resultand use?instead of panicking.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/surfaces/visualization/plotters.rs | Updates doc example to use Result + ? instead of .expect()/panic cleanup. |
| src/surfaces/surface.rs | Removes unwraps in sorting/interpolation/extrema paths; adds explicit indexing panic message; introduces warn!-based fallbacks. |
| src/curves/visualization/plotters.rs | Updates doc example to use Result + ? instead of .expect()/panic cleanup. |
| src/curves/visualization/mod.rs | Updates module-level doc examples to use Result + ?. |
| src/curves/traits.rs | Converts to_f64().unwrap() conversions in regression generation into typed CurveError returns. |
| src/curves/curve.rs | Replaces indexing .expect() with explicit panic!; removes unwraps from metric/extrema helpers via ok_or_else(...)? and guarded comparisons. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…es + clearer to_f64 error - src/surfaces/surface.rs::cubic_interpolate: when sqrt() returns None (Decimal overflow on very large operand), early-return Decimal::ZERO weight to drop the point's contribution rather than treating it as zero-distance (which would have made it look infinitely close). - src/surfaces/surface.rs::get_closest_point: compare squared distances directly instead of taking sqrt; ordering is monotonic on non-negative inputs, so the sqrt is unnecessary and would otherwise introduce a fallback that could distort ordering. - src/curves/traits.rs: reword to_f64 ok_or_else messages — Decimal has no NaN/Inf, so the failure mode is out-of-range / non-representable.
Owner
Author
|
Thanks for the review. All four inline findings are addressed in 2f9925d. |
7 tasks
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
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.
Summary
.unwrap()/.expect()in production (non-#[cfg(test)]) code undersrc/curves/andsrc/surfaces/per the M1 — Panic-Free Core milestone.src/curves/curve.rs(18),src/surfaces/surface.rs(25),src/curves/traits.rs(2), and 4 doc-comment examples in the visualization modules.CurveError::AnalysisError/InterpolationError/invalid_parametersandSurfaceError::AnalysisError/invalid_parameterscover every case.Index<usize>::indexkeeps the trait-mandated panic on out-of-bounds but uses an explicitpanic!withindexandleninstead of.expect("Index out of bounds")(satisfies the panic-free grep, identical user-facing behaviour).Per-pattern transformations applied
Decimal::partial_cmp(...).unwrap()in sort/min_by/max_by closuresunwrap_or(std::cmp::Ordering::Equal)(NaN/Inf guard perglobal_rules.md§Numerical Discipline)min_by/max_by(...).unwrap()Option chains in Result-returning fnsok_or_else(|| <Module>Error::AnalysisError(...))+?iter().nth(index).expect("Index out of bounds")inIndex::indexpanic!("...index = N, len = M")+# PanicsdocDecimal::sqrt().unwrap()in Result-returning fnsok_or_else(|| SurfaceError::invalid_parameters("sqrt", ...))+?Decimal::sqrt().unwrap()in non-Result closures (sort comparators)unwrap_or_else(|| { tracing::warn!(...); Decimal::ZERO })(operands are non-negative; failure path is theoretical overflow)to_f64().unwrap()ok_or_else(|| CurveError::invalid_parameters("to_f64", ...))+?get_point(x).unwrap()aftercontains_point()invariantok_or_else(|| <Module>Error::InterpolationError(...))+?.expect("panic message")examples# fn run() -> Result<(), Box<dyn Error>>shim, propagate via?Test plan
awk '/#\[cfg\(test\)\]/{exit} {print}' <file> \| grep -E '\.unwrap\(\)\|\.expect\('acrosssrc/curves/**/*.rsandsrc/surfaces/**/*.rscargo clippy --all-targets --all-features --workspace -- -D warningscargo fmt --all --checkcargo test --all-features --workspace(3724 lib + 416 integration + plotly)cargo build --releasemake pre-push(fix + fmt + lint-fix + test + readme + doc)Public-surface notes
Curve::index/Surface::indexkeep the sameIndextrait contract (panic on OOB) and document the panic in# Panics.README.tplregen bymake pre-pushproduced no surface-level diff.Closes #319