Move code out of add_entry! in get_route
#2920
Conversation
WalkthroughThe recent update involves a significant refactoring effort within the routing logic of the application. A complex macro previously embedded in the code has been elegantly extracted and encapsulated into a new function named Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2920 +/- ##
==========================================
- Coverage 89.80% 89.69% -0.12%
==========================================
Files 121 121
Lines 100045 100039 -6
Branches 100045 100039 -6
==========================================
- Hits 89845 89728 -117
- Misses 7533 7623 +90
- Partials 2667 2688 +21 ☔ View full report in Codecov by Sentry. |
Velnbur
force-pushed
the
move-add-entry-macro-in-get-route
branch
from
de2a0b7 to
e85a59a
Compare
|
Hmmm, I'm definitely a fan of moving code out of get_route, but passing in 28 arguments doesn't really improve readability :/. For panics specifically, luckily the panic messages share the arguments passed to the panic, so its still not that hard to just search for the specific panic we hit (and we can add relevant messages to any panics that don't have such clarity). For generally cleaning up routing, if you want to continue down this path we'll need to try to build state structs that we can pass around as singular objects rather than individual fields. |
Yeah, that's a great point, and capturing them through macro makes it hard to read as well. Just wanted to make it "compatible" with other code that's written in parallel with this PR.
I wanted to hear that, so if there is some interest in it, I may continue P.S.: Also it seems that the |
|
The other issue we're gonna hit is the routefinding rewrites in #2803 are going to conflict heavily here :/ |
|
Okay...the bulk of #2803 landed. Any desire to pick this back up? |
Velnbur
force-pushed
the
move-add-entry-macro-in-get-route
branch
from
e85a59a to
4e3f404
Compare
|
Rebased.
Yes, if there is still any interest in it.
I'll start from this then. Beginning with obvious ones like |
|
Still need to add something for the "state" of the path-finding algorithm, but even at this point I'll be glad for any comments about the direction of the changes |
Move all of the code out of `add_entry!` macro inside `get_route` to facilitate development experience inside it and reduce enourmous `get_route` function size.
Velnbur
force-pushed
the
move-add-entry-macro-in-get-route
branch
from
9a94b41 to
dfb03aa
Compare
Velnbur
force-pushed
the
move-add-entry-macro-in-get-route
branch
from
dfb03aa to
bc51298
Compare
Group const after initialization values used across the `get_route` method into single `GetRouteParameters` struct.
To `NextHopsData` and use it instead everywhere possible
Velnbur
force-pushed
the
move-add-entry-macro-in-get-route
branch
from
bc51298 to
b4c62dc
Compare
|
Nice, yea, in principle this looks great to me, assuming benchmarks don't turn up anything weird (might well be faster cause improved code density). |
|
Closing as abandoned. |
Move all of the code out of
add_entry!macro insideget_routeto facilitate development experience inside it (as by this time macro backtrace is still only available in nightly versions of compiler) and reduce enourmousget_routefunction size.