feat: add fallible variants for dijkstra and yen algorithms #689
Conversation
WalkthroughThe Dijkstra and Yen algorithm implementations were refactored to introduce fallible variants that propagate errors from user-supplied closures. New Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dijkstra
participant UserClosure
User->>Dijkstra: try_dijkstra(start, successors, success)
Dijkstra->>UserClosure: successors(node) returns Result
alt Ok
Dijkstra->>UserClosure: success(node) returns Result
alt Ok
Dijkstra-->>User: Result<Option<(Path, Cost)>, E>
else Error
Dijkstra-->>User: Err(E)
end
else Error
Dijkstra-->>User: Err(E)
end
sequenceDiagram
participant User
participant Yen
participant Dijkstra
participant UserClosure
User->>Yen: try_yen(start, successors, success, k)
loop up to k times
Yen->>Dijkstra: dijkstra_internal(...)
Dijkstra->>UserClosure: successors(node) returns Result
Dijkstra->>UserClosure: success(node) returns Result
Dijkstra-->>Yen: Result<Option<(Path, Cost)>, E>
Yen-->>User: Result<Vec<(Path, Cost)>, E>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/directed/yen.rs (1)
113-121: Consider using a more idiomatic unwrap pattern.Since
Infalliblecan never be constructed, the match statement could be simplified.- match try_yen::<_, _, _, _, _, Infallible>( - start, - |node| Ok(successors(node)), - |node| Ok(success(node)), - k, - ) { - Ok(v) => v, - } + try_yen::<_, _, _, _, _, Infallible>( + start, + |node| Ok(successors(node)), + |node| Ok(success(node)), + k, + ) + .unwrap_or_else(|inf: Infallible| match inf {})This makes it explicit that the error case is impossible and will satisfy the compiler's exhaustiveness checking.
src/directed/dijkstra.rs (3)
83-91: Consider using the same idiomatic unwrap pattern as suggested for yen.rs.For consistency across the codebase and better idiomaticity.
-{ - match dijkstra_internal::<_, _, _, _, _, Infallible>( - start, - &mut |node| Ok(successors(node)), - &mut |node| Ok(success(node)), - ) { - Ok(result) => result, - } -} +{ + dijkstra_internal::<_, _, _, _, _, Infallible>( + start, + &mut |node| Ok(successors(node)), + &mut |node| Ok(success(node)), + ) + .unwrap_or_else(|inf: Infallible| match inf {}) +}
229-236: Apply the same unwrap pattern for consistency.- match try_dijkstra_partial::<_, _, _, _, _, Infallible>( - start, - |node| Ok(successors(node)), - |node| Ok(stop(node)), - ) { - Ok(result) => result, - } + try_dijkstra_partial::<_, _, _, _, _, Infallible>( + start, + |node| Ok(successors(node)), + |node| Ok(stop(node)), + ) + .unwrap_or_else(|inf: Infallible| match inf {})
487-515: Verify: Should dijkstra_reach have a fallible variant?The
dijkstra_reachfunction and its iterator implementation still use infallible closures. Was this intentionally left out of the fallible variants update?If a fallible variant is needed, I can help implement
try_dijkstra_reach, though it would require careful consideration of how to handle errors in the iterator pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/directed/dijkstra.rs(7 hunks)src/directed/yen.rs(5 hunks)
🔇 Additional comments (9)
src/directed/yen.rs (3)
8-8: Import addition looks good.The
Infallibletype is correctly imported for use in the infallible wrapper pattern.
123-229: Excellent implementation of the fallible variant.The error propagation is correctly implemented throughout the function, maintaining the original algorithm logic while properly handling potential failures in the
successorsandsuccessclosures.
231-247: Helper function correctly updated for error propagation.The
make_costfunction properly handles the falliblesuccessorsclosure and propagates errors as expected.src/directed/dijkstra.rs (6)
11-11: Import correctly added.The
Infallibletype import is necessary for the wrapper pattern implementation.
93-112: Clean implementation of the fallible dijkstra variant.The function correctly delegates to
dijkstra_internalwith proper error type constraints.
114-133: Internal function correctly updated for error handling.The
dijkstra_internalfunction properly propagates errors fromrun_dijkstrawhile maintaining the original logic.
183-199: Fallible dijkstra_all variant correctly implemented.The function properly handles errors and delegates to
try_dijkstra_partialwith appropriate error propagation.
238-239: Good use of type aliases for clarity.The
PartialResultandRunResulttype aliases improve code readability.Also applies to: 271-272
273-333: Core dijkstra implementation correctly updated for error handling.The
run_dijkstrafunction properly propagates errors from bothstopandsuccessorsclosures while maintaining the algorithm's correctness.
CodSpeed Performance ReportMerging #689 will degrade performances by 18.38%Comparing Summary
Benchmarks breakdown
|
|
doesn't look like the regression is related to this change, considering |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation