-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(router): add merge function #62
Conversation
Signed-off-by: Andrey Mak <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this! A couple small comments but the general direction looks good. I agree a suboptimal implementation performance wise is not a dealbreaker — it can always be optimized later.
- do not mutate child router - return list of insert errors - call visitor on each node with value Signed-off-by: Andrey Mak <[email protected]>
Signed-off-by: Andrey Mak <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good to go with a couple smaller changes.
- Consume child node to take values - Add MergeError to satisfy error trait Signed-off-by: Andrey Mak <[email protected]>
- initialize queue from known-size set - push back childs to make algo true BFS - adjust test cases Signed-off-by: Andrey Mak <[email protected]>
Looks good now, thanks! |
Closes #57
I am sure it is not the best way to implement such functionality, but since usually routes are defined at server startup there should be no significant overhead.