Open CathalMullan opened 2 days ago
codspeed-hq[bot]
commented
2 days ago Comparing 163-consider-adding-support-for-inline-wildcards (eda75c0) with main (fa8d921)
✅ 19 untouched benchmarks
codecov[bot]
commented
2 days ago Attention: Patch coverage is 80.00000% with 21 lines in your changes missing coverage. Please review.
:white_check_mark: All tests successful. No failed tests found.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/node/search.rs | 73.84% | 11 Missing and 6 partials :warning: |
| src/node/optimize.rs | 84.61% | 4 Missing :warning: |
| Files with missing lines | Coverage Δ | |
|---|---|---|
| src/node.rs | 92.72% <ø> (ø) |
|
| src/node/insert.rs | 94.37% <100.00%> (+0.15%) |
:arrow_up: |
| src/router.rs | 85.71% <100.00%> (+0.07%) |
:arrow_up: |
| src/node/optimize.rs | 92.75% <84.61%> (-5.08%) |
:arrow_down: |
| src/node/search.rs | 78.02% <73.84%> (-1.18%) |
:arrow_down: |
Test suite is lacking.
We have no way of measuring performance of wildcards currently. Need benches.
Use Codecov to guide what test cases we need to add.
Should really look at how Rails handles these types of scenarios: https://github.com/rails/rails/tree/main/actionpack
ANSWER: Insert order matters. We don't want that to be the case here.
Maybe our priority should be first branch wins, then deepest match? Would this cause issues with new routes being inserted over time? Is our current quick dynamic susceptible to this issue?
Maybe add a 'Limitations' section to the README, mentioning we don't try and prevent unreachable routes? And mention our walking approach.
Having 2 or more dynamics/wildcards at the same route level is the issue.
Maybe having an optional tracing feature would be nice for matching, if out logic gets complicated. Would help with debugging too. Maybe spin out to a seperate ticket.
Currently this walks inline wildcards byte-by-byte, which is likely slow.
Copied the segment vs inline naming from dynamic, but technically isn't right.