Skip to content
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

Merged
merged 5 commits into from
Oct 14, 2024
Merged

Conversation

darkenmay
Copy link
Contributor

@darkenmay darkenmay commented Oct 11, 2024

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.

Copy link
Owner

@ibraheemdev ibraheemdev left a 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.

src/router.rs Outdated Show resolved Hide resolved
src/router.rs Show resolved Hide resolved
src/tree.rs Outdated Show resolved Hide resolved
src/tree.rs Outdated Show resolved Hide resolved
- do not mutate child router
- return list of insert errors
- call visitor on each node with value

Signed-off-by: Andrey Mak <[email protected]>
Copy link
Owner

@ibraheemdev ibraheemdev left a 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.

src/router.rs Outdated Show resolved Hide resolved
src/router.rs Show resolved Hide resolved
src/tree.rs Outdated Show resolved Hide resolved
src/tree.rs Outdated Show resolved Hide resolved
- 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]>
@ibraheemdev
Copy link
Owner

Looks good now, thanks!

@ibraheemdev ibraheemdev merged commit 8e66ae9 into ibraheemdev:master Oct 14, 2024
5 checks passed
@darkenmay darkenmay deleted the merge branch October 14, 2024 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow merging two matchit Routers
2 participants