Conversation
Move RouterId and router_ids from router_manager into factory to keep the dependency direction one-way (router_manager → factory). Add RouterFactory::create_igw_routers() that returns all IGW routers as (id, label, result) tuples, and RouterManager::try_register() that handles the match/log pattern. The IGW block in from_config() goes from ~90 lines of repeated match/register/log to a simple loop. Adding a new router to IGW mode now requires a single line in create_igw_routers() instead of copying an 8-line block in a separate file. Files changed: - model_gateway/src/routers/factory.rs: add RouterId, router_ids module, and create_igw_routers() method - model_gateway/src/routers/router_manager.rs: remove RouterId and router_ids (now imported from factory), add try_register() helper, replace 6 repeated match blocks with loop over create_igw_routers()
Summary of ChangesHello @slin1237, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the router initialization process within the Inter-Gateway (IGW) mode. By centralizing router creation logic and streamlining the registration boilerplate, it enhances code maintainability and simplifies the future addition of new routers. The changes also improve the module dependency structure, ensuring a clearer and more logical flow of information between components. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughRouter creation logic has been refactored to centralize multi-router setup. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a solid refactoring that significantly reduces boilerplate for registering IGW routers. The introduction of create_igw_routers in the factory and the try_register helper in the manager centralizes logic, making the code cleaner and more maintainable. Moving RouterId to the factory module is also a good move to fix the dependency direction. Overall, this is a great improvement. I have one minor suggestion to make the new RouterId type more ergonomic to use.
| impl RouterId { | ||
| pub const fn new(id: &'static str) -> Self { | ||
| Self(id) | ||
| } | ||
|
|
||
| pub fn as_str(&self) -> &str { | ||
| self.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
To make RouterId more ergonomic, consider implementing the Display and Deref traits. This would allow you to use it directly in formatting macros (like info!) and other places expecting a &str, removing the need for the .as_str() method.
After applying this suggestion, you can remove the .as_str() calls in router_manager.rs.
impl RouterId {
pub const fn new(id: &'static str) -> Self {
Self(id)
}
}
impl std::fmt::Display for RouterId {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.0)
}
}
impl std::ops::Deref for RouterId {
type Target = str;
fn deref(&self) -> &Self::Target {
self.0
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
2 participants
Summary
Reduces ~90 lines of repeated match/register/log boilerplate in
RouterManager::from_config()down to a simple loop, and fixes the module dependency direction sorouter_managerdepends onfactory(not circular).What changed
model_gateway/src/routers/factory.rs:RouterIdtype androuter_idsconstants here fromrouter_manager.rs— factory is the lower-level module, so manager should depend on it, not the other way aroundcreate_igw_routers()method returning all 6 IGW routers as(id, label, Result)tuplesmodel_gateway/src/routers/router_manager.rs:RouterIdandrouter_idsdefinitions (now imported fromfactory)try_register()helper that handles the match/log pattern for a single routerfrom_config()with a loop overcreate_igw_routers()RouterFactoryimport from function-level to module-levelWhy
Adding a new router to IGW mode previously required copying an 8-line block in
router_manager.rsand updating 3 strings. Now it requires a single line increate_igw_routers()infactory.rs— the same file where thecreate_*_router()methods live.How
The key insight is that the 6 router creation blocks in
from_config()all followed the exact same pattern: call a factory method, match on Ok/Err, log, and register. By having the factory return all results as a vec of tuples, the manager can loop over them with a generictry_register()helper.RouterId/router_idsmoved tofactory.rsto keep the dependency one-directional.Test plan
cargo build— compiles cleanlycargo clippy -p smg --all-targets --all-features -- -D warnings— no warningscargo test -p smg— all 16 tests pass, 0 failuresSummary by CodeRabbit