CaptureAll #1516
CaptureAll #1516
Conversation
* This fails two of the tests. We should probably add a catch all
at the end that captures anything not matches by the Symbols for
the preceding branches `arms` and `legs`.
* When using `CaptureAll` from the root "/" and empty path becomes
an empty list. when using it from a branch though an empty path,
ie. a trailing separator, becomes `mempty` for the type. Where is
this happening? I think this is definitely happening before
`parseUrlpieces` gets run, so it happens in the machinery for
Delayed and DelayedIO. I am still looking for the code that
produces the `txts` argument for the `captureD` lambda created in
the `HasServer` instance for `CaptureAll`.
* Routes ending in a `CaptureAll` now get an empty list instead of
[""] when they have a trailing slash.
* WARNING: I think this will break the expectation that a rooted
capture all will produce [""] for `//`. That is, previously `// =>
[""]`, but I think `// => []`. I will make some tests to check
and see what is going on with this.
* Trailing slashes will no longer affect the captures for "rooted"
CaptureAll apis (test included).
guygastineau
force-pushed
the
captureall
branch
from
8976ce7
to
2e5fe27
Compare
|
@domenkozar please let us know if this solves your issue. |
|
This PR does change the data processed by handlers for CaptureAll apis. This might mean some kind of breaking change. The difference is: |
|
I forgot to make a file in changeling.d. sorry |
|
FWIW, The new behaviour seems more correct to me. |
|
@alpmestan great. The side by side string and int tests might be redundant. Or maybe the community wants new tests? I think I got the tests we need to understand my change and prevent regression, but I'd love if someone tried to poke holes in that confidence. |
changelog.d/CaptureAll
Outdated
|
|
||
| description: { | ||
|
|
||
| CaptrueAll resulted in `[""]` for empty paths due to trailing slash. Similar |
There was a problem hiding this comment.
Thanks you. I'll get the typo fixed this afternoon EST.
There was a problem hiding this comment.
Oh, I forgot to get to this. I'll get it today.
There was a problem hiding this comment.
I amended and force pushed. Apologies to anyone who might have fetched my branch.
guygastineau
force-pushed
the
captureall
branch
from
d1fb282
to
ca86b49
Compare
|
CI jobs running now. If it goes through, this is IMO good to go. @akhesaCaro @gdeest @tchoutri Your opinion? |
| @@ -178,7 +178,10 @@ runRouterEnv fmt router env request respond = | |||
| -> let request' = request { pathInfo = rest } | |||
| in runRouterEnv fmt router' (first, env) request' respond | |||
| CaptureAllRouter router' -> | |||
| let segments = pathInfo request | |||
| let segments = case pathInfo request of | |||
| -- this case is to handle trailing slashes. | |||
|
Is there a reason why this hasn't been merged yet other than everyone being busy and letting slip through the cracks? It has merge conflicts now. Should I fix that on my end? |
closes #1243
In the end it was a simple fix, but new tests were necessary. I also modified the existing tests for the original test API for CaptureAll. Using the sum of a list let the bug from #1243 linger. Now the tests will catch any regressions. I am not sure if there.
I tried combining the "rooted" capture all API at the end of the legs and arms API, but then it caught the parsing failure tests for the leg route, so I figured it might as well be another router.
I tried to be tidy.